Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grass.temporal.datetime_math: Return a TypedDict from compute_datetime_delta #4700

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

echoix
Copy link
Member

@echoix echoix commented Nov 14, 2024

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-syntax

Either 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 month key 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,
image
After going through the month logic:
image
We get warned that month can be not set when used
image

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.

@echoix echoix requested a review from petrasovaa November 14, 2024 04:33
@github-actions github-actions bot added Python Related code is in Python libraries labels Nov 14, 2024
@echoix echoix force-pushed the typing-temporal-part2 branch from bc30daa to 8c24335 Compare November 14, 2024 04:38
…e_delta and datetime_delta dict

Removed corresponding tests from the deprecated python/grass/temporal/unit_tests.py file which isn't run
@echoix echoix force-pushed the typing-temporal-part2 branch from 8c24335 to 6246186 Compare November 14, 2024 04:43
@OSGeo OSGeo deleted a comment from github-actions bot Nov 14, 2024
@echoix
Copy link
Member Author

echoix commented Nov 14, 2024

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

@echoix
Copy link
Member Author

echoix commented Nov 14, 2024

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

@echoix
Copy link
Member Author

echoix commented Nov 14, 2024

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.

@echoix
Copy link
Member Author

echoix commented Nov 14, 2024

The generated docs link to the TypedDict class:
image
image

image

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).

@echoix
Copy link
Member Author

echoix commented Nov 14, 2024

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

@wenzeslaus
Copy link
Member

Looks good in general, but what's up with all the underscores?

@echoix
Copy link
Member Author

echoix commented Nov 14, 2024

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).

@echoix
Copy link
Member Author

echoix commented Nov 15, 2024

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)

@echoix
Copy link
Member Author

echoix commented Nov 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libraries Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants