Skip to main content
added 24 characters in body
Source Link
J_H
  • 34.9k
  • 3
  • 32
  • 129
  1. They compose nicely, e.g. Mon .. Wed combined with Wed .. Sat gives all weekdays.
  2. They match the builtin range semantics.
  3. Magic numbers appear in their more obvious form, e.g. 59 vs 60.
  4. Continuous and discrete work the same (float and int, also datetime and date).
  1. They compose nicely, e.g. Mon .. Wed combined with Wed .. Sat gives all weekdays.
  2. They match the builtin range semantics.
  3. Magic numbers appear in their more obvious form, e.g. 59 vs 60.
  4. Continuous and discrete work the same (float and int).
  1. They compose nicely, e.g. Mon .. Wed combined with Wed .. Sat gives all weekdays.
  2. They match the builtin range semantics.
  3. Magic numbers appear in their more obvious form, e.g. 59 vs 60.
  4. Continuous and discrete work the same (float and int, also datetime and date).
deleted 2 characters in body
Source Link
J_H
  • 34.9k
  • 3
  • 32
  • 129

It would be very nice if you explain we're expecting str input rather than a datetimedatetime.time:

Your H,M,S tuple unpack makes me slightly nervous, as it's reading from a dict rather than a tuple. Yes, in cPython since 3.5 we've had stable iterator order which matches the key insertion order. But the python language is bigger than cPython, there are other interpreters. It just seems slightly jarring that we don't have three dict de-refs there. Feel free to keep it, anyway. Consider turning H,M,S into a namedtuple or dataclass. Or better, prefer to use a datetimetime.

If datetimesdatetime times were involved we wouldn't be jumping through hoops like this.

It would be very nice if you explain we're expecting str input rather than a datetime:

Your H,M,S tuple unpack makes me slightly nervous, as it's reading from a dict rather than a tuple. Yes, in cPython since 3.5 we've had stable iterator order which matches the key insertion order. But the python language is bigger than cPython, there are other interpreters. It just seems slightly jarring that we don't have three dict de-refs there. Feel free to keep it, anyway. Consider turning H,M,S into a namedtuple or dataclass. Or better, prefer to use a datetime.

If datetimes were involved we wouldn't be jumping through hoops like this.

It would be very nice if you explain we're expecting str input rather than a datetime.time:

Your H,M,S tuple unpack makes me slightly nervous, as it's reading from a dict rather than a tuple. Yes, in cPython since 3.5 we've had stable iterator order which matches the key insertion order. But the python language is bigger than cPython, there are other interpreters. It just seems slightly jarring that we don't have three dict de-refs there. Feel free to keep it, anyway. Consider turning H,M,S into a namedtuple or dataclass. Or better, prefer to use a time.

If datetime times were involved we wouldn't be jumping through hoops like this.

Source Link
J_H
  • 34.9k
  • 3
  • 32
  • 129

Thank you for the obvious effort put into readability.

isort

Your imports are nice enough as is. But you're working too hard. Run "$ isort *.py" and be done with it, don't give it a second thought. The package's motto is "I sort ... so you don't have to."


config

Gosh, that's a lot of config details! Reading this for the first time, I'm wondering if maybe it's not the most convenient way to achieve your goal. There's lots of key names that appear here as str and will wind up being bound to identifiers down in the code. Feel free to keep it as-is, but I will toss out a pair of alternatives to consider. And the three Hands makes me wonder if we'd like a class or a named tuple to represent them.

This dict may look like data but really it's code, within a source file. Consider using json.dump(), or better, yaml.dump(), to serialize it out to a config file. Use the same git source control edit / commit practices on the config file that you use on your *.py source files.

Consider using keyword arguments (kwargs) in a function's signature to adjust some of these parameters. And then typer gives you CLI --help "for free".

def main(
        nav_title="Navigation Window",
        nav_label_text="Validate time value (HH:MM:SS)",
        theme_light_bg="white",
        theme_dark_bg="black",   # and so on...
    ):
    ...

if __name__ == "__main__":
    typer.run(main)

comments

There's nothing wrong with writing comments -- feel free to do so.

But sometimes, if you feel a need to explain code which isn't sufficiently clear, the comment is a code smell that is helping you to identify a refactor.

# VALIDATION LOGIC


# Validation handler
def handle_validation():
    ...

# Function to validate the user's input
def validate_time(user_time, ...

All of those identifiers are great.

The "validation logic" comment is maybe helpful for grouping things, maybe not too redundant with the "valid"s we see in function names. Or maybe it would be helpful to banish these functions to a new validation.py module, and import them. Similarly for a draw_clock.py module.

The next two comments are on the redundant side, enough that I'd probably delete one or both of them. If you do wish to retain descriptions of those (well named, self explanatory!) functions, then use a """docstring""" rather than a # comment. A single English sentence would likely suffice.

It would be very nice if you explain we're expecting str input rather than a datetime:

def validate_time(user_time: str, ...

Now, about that second argument, the state dict. Consider creating a class so you can turn this function into a method.

    def validate_time(self, user_time: str):

Then we might access self.state. Or we might break out its values to store each one directly as an object attribute.

I confess that referencing state.values() confuses me a bit. I was expecting to see state.current_time.values().

Your H,M,S tuple unpack makes me slightly nervous, as it's reading from a dict rather than a tuple. Yes, in cPython since 3.5 we've had stable iterator order which matches the key insertion order. But the python language is bigger than cPython, there are other interpreters. It just seems slightly jarring that we don't have three dict de-refs there. Feel free to keep it, anyway. Consider turning H,M,S into a namedtuple or dataclass. Or better, prefer to use a datetime.

BTW, kudos on that map(int, user_time.split(":")) tuple unpack expression. It's a very clear, natural way to express your intent.

Give the signature a type annotation, please, so caller knows to expect a tuple[bool, str].


diagnostic message

        return False, "Invalid format (HH:MM:SS)"

This is nice as-is. Consider tacking on the bad user_time value, to help some future maintenance engineer diagnose bugs quicker.

Rather than returning a boolean, you possibly would like to raise ValueError(...), so caller can't accidentally ignore the False value.


half-open interval

Kudos for the very clear math notation in this expression:

    if not (... and 0 <= user_second <= 59):

You might want to habitually prefer a half-open over a closed interval: 0 <= user_second < 60 There are several advantages.

  1. They compose nicely, e.g. Mon .. Wed combined with Wed .. Sat gives all weekdays.
  2. They match the builtin range semantics.
  3. Magic numbers appear in their more obvious form, e.g. 59 vs 60.
  4. Continuous and discrete work the same (float and int).

nit: Here's a fun fact. The <= 59 expression isn't exactly correct. Twice a year (start of January & July) a leap second may be inserted. Which lets 23:59:59 tick over to 23:59:60 before advancing to 00:00:00. Many people (well, non-astronomer people) ignore this, and you should, too. The takeaway is "timekeeping is complex", and you'd be better off modeling instants of time with a mature library such as datetime.


modulo

    elif ( ...
        (
            user_hour == current_hour
            or user_hour - 12 == current_hour
            or user_hour == current_hour - 12
            or (user_hour in [0, 24, 12] and current_hour == 0)
        )

Yikes! Such a simple concept, so much complexity. It looks like you had trouble with edge cases and just kept piling one fix on top of another.

Step back a moment. The math concept you're looking for is % modulo, which grade school math teachers often introduce in terms of clock hands. Compare both quantities mod 12 and we're done.

If datetimes were involved we wouldn't be jumping through hoops like this.

Maybe we're sad that we lost the AM/PM indicator bit? Better, maybe we should have immediately trimmed twelve hours off any PM input values, before it could even get here.


@property

The FOO_angle functions are very nice, keep them as-is. Perhaps mark them as _private helpers with a leading _ underscore. In general, your Public API is pretty big, exporting a great many identifiers.

I will just point out that if you create your own class (maybe a dataclass) to represent an instant in time, there is an opportunity to turn those functions into three @property decorated methods.


negation

def toggle_theme could toggle by assigning not state["theme"]["light_mode"].

Notice that we could construct the relevant key name with an expression like:

>>> ['dark', 'light'][False]
'dark'
>>> ['dark', 'light'][True]
'light'

main guard

root = Tk()

This is bog standard, it's nice as-is.

Please bury it within def main(): or another function. That way other modules, such as a test suite, can import this one and exercise the various helper functions.

It is usual that import should not print, create a new window, or have other annoying side effects, beyond defining functions and constants.

You have a bunch of helpers. They would benefit from an automated unit test suite.


unyt

There's no need to adopt this library. But I will note in passing that unyt lets you annotate your variables with degree or radian. This can promote type safety and reduce confusion.


This well-engineered application achieves its design goals.

I would be willing to delegate or accept maintenance tasks on it.