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.
- They compose nicely, e.g. Mon .. Wed combined with Wed .. Sat gives all weekdays.
- They match the builtin range semantics.
- Magic numbers appear in their more obvious form, e.g.
59
vs 60
.
- 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.