grass.temporal.datetime_math: Return a TypedDict from compute_datetime_delta #4700
Conversation
…e day_diff` instead of `if`-`else`-block
echoix
force-pushed
the
typing-temporal-part2
branch
from
bc30daa
to
8c24335
Compare
…e_delta and datetime_delta dict Removed corresponding tests from the deprecated python/grass/temporal/unit_tests.py file which isn't run
echoix
force-pushed
the
typing-temporal-part2
branch
from
8c24335
to
6246186
Compare
|
I see that I mistakenly committed some changes that were intended for another branch/PR when amending in the file python/grass/temporal/datetime_math.py |
Reverted them |
|
I did a commit to remove that highlights the tests that depend on the month value being initialized to a value, by changing the initialized value to 110. |
|
The generated docs link to the TypedDict class:
Note that the order of the docs is changed to be alphabetical. It is not an OrderedDict, order doesn't matter (even if dicts in 3.7 are guaranteed to keep insertion order). |
|
Note: Currently pytest wasn't configured to accept the default naming scheme, so my purposely "broken" commit didn't fail. I probably won't have time today for this kind of edit |
|
Looks good in general, but what's up with all the underscores? |
A typed dict needs to have all keys filled, and we can create them with a dict literal. This means I couldn't have it empty, add a key, and use the existing values to build a next value for another key. It needed to be set at once. So I used local variables for the internal variables, and didn't want to collide with special names (like min). |
|
Ok, please now take a look at the tests I made sure to make failing. These are the ones that did not set any value for the "month" key before. I made the tests by setting the initial value to 0, and then changed it to another value (110) so we would see were the 110 value leaked. Tests didn't run before (only possibly doctest, but they are not enforced everywhere as they fail) |
|
If I could have a review on the logic side (ie, what should the function return) before the weekend, I would have the opportunity to fix it there and anyone could review the Python part. |
This PR started by wanting to annotate
compute_datetime_delta, which ended up needing a TypedDict to fully describe the keys that are returned and can't be omitted. A TypedDict can be defined in two ways, with a class syntax or functionnal syntax. In both cases, at runtime it is a plain dict, and the class syntax doesn't really define a class, as no values can be assigned, only annotations. https://mypy.readthedocs.io/en/stable/typed_dict.html#class-based-syntaxEither way, after editing the lines so that the dictionary can be initialized only once with all required keys, I found a hole in the logic that makes it possible for the
monthkey to not always be assigned. Correctly factorized, linters or type checkers (pylint and pylance this time), correctly identify the same issue I found earlier:By commenting out the variable initializations,
After going through the month logic:
We get warned that month can be not set when used
I added an extra test case for this. We can also see the effects on the tests if we use a value different from 0 for the initialization value of months, like 110, where multiple tests will now fail.
So what should be the correct logic here?
Lastly, some tests already existed, but in a file "unit_tests.py" that doesn't seem to run, and the file header mentions they are deprecated. So, I copied the test inputs over to a new pytest test (as it is a python-only method). Since these tests only checked for one (or sometimes more) values from the dict, I tried to calculate the expected values manually without looking at the doctests of the function. Even at the end, I was still making mistakes on when a key should contain the total of accumulated time vs 0. So maybe take another fresh look there.