Typing Improvements #67
Conversation
In process of fixing type hints in constructor function had to refactor slightly, less repeated dictionary assignment and instead just create a dictionary with required keys and values in the final step before assigning to the groupname
Plus refactors to dictionary constructor function
Implementation here could be made neater by building the output dict from a fixed input dict
…n functions, remove OrderedDict Dictionary ordering guranteed in python 3.7+ anyway so not required. Type hinted group data dictionaries obliviate need to manually assign type of class_data
floatarray type enough to show that a single handicap in gives single handicap values back out
Use literal type to constrain allowable parameter values for any funciton constructing a Target instance
…isting Use typing.get_args to deduplicate list of allowable scoring systems
|
Apologies, didn't realise my main branch was behind, merge conflicts fixed on my end but worth a double check. |
There was a problem hiding this comment.
Thanks @TomHall2020 this is great!
A couple of queries/comments and lines I think may now be redundant.
Apologies for not telling you about the docs - I had it on my mind to drop you a message saying you'd want to rebase, but forgot. -_- PS - I wrote some docs!
If you could look at the comments, amend and push with formatting and linting that'd be great. On a side note, how would you feel about using pre-commit on the repo. In general I'm not that keen on it, but I can see it could be useful on this project?
General queries:
Changes from Union[int, float] to float are because python is 'clever enough' to take an integer input and just convert it to a float, so as long as we always use floats internally mypy doesn't complain? I think I'm still thinking in Fortran/C after a week of work.
I wonder if the ScoringSystem type should live in utils or target.
I think I err on it being in target as you have done given it is directly associated with targets.
|
Oh, and python 3.9 doesn't have |
|
Regarding the JSON, yes, it had crossed my mind that reading every time isn't ideal, but it's not a bottleneck. |
jatkinson1000
added
documentation
Good spot. It's not necessary, just more explicit. We could install typing_extensions and get access to it, as well as other goodies like |
I've never had to use it before but aware of it; I can't really offer any experience for an opinion, but I guess the question is the benifit of keeping every commit clean worth forcing all future contributors be comfortable/able to use it before they can add anything to the project?
Less that it converts it to a float but just that ints are compatible with operations on floats. mypy docs explain it, essentially so long as you don't explicitly call any float only methods you are generally safe to assume ints are usable anywhere floats are.
Agree here, makes sense to keep it close to where its used, and importable from the same place as Target (any consumer wanting to write a type checked function that creates Targets would need the ScoringSystem type too). |
Label for reintroduction in py 3.10+
Broadened type to accept more than a list of passes
Makes sense, I didn't consider those files being read in by other projects. Out of scope for this but perhaps a good compromise is to just call the read_json functions once in a module (either classification_utils or a seperate classifcation_constants), assign the results to constants and then import those in each of the modules they are used. Actually has the side benifit of moving those to global variables rather than locals in the _make_classification_dict functions. |
Older version did not correct module docstring spacing
|
Thats annoying, I did run black to generate the final commit there, but it didn't fix the spacing of module docstrings to imports on my local version ❯ black archeryutils
reformatted /Users/Tom/dev/forks/archeryutils/archeryutils/classifications/agb_field_classifications.py
reformatted /Users/Tom/dev/forks/archeryutils/archeryutils/classifications/agb_outdoor_classifications.py
reformatted /Users/Tom/dev/forks/archeryutils/archeryutils/rounds.py
reformatted /Users/Tom/dev/forks/archeryutils/archeryutils/targets.py
reformatted /Users/Tom/dev/forks/archeryutils/archeryutils/handicaps/handicap_equations.py
reformatted /Users/Tom/dev/forks/archeryutils/archeryutils/tests/test_rounds.py
reformatted /Users/Tom/dev/forks/archeryutils/archeryutils/tests/test_targets.py
reformatted /Users/Tom/dev/forks/archeryutils/archeryutils/handicaps/tests/test_handicaps.py
All done! ✨ 🍰 ✨
8 files reformatted, 21 files left unchanged.
❯ which black
/Users/Tom/dev/forks/archeryutils/.venv/bin/black
❯ black --version
black, 23.12.1 (compiled: yes)
Python (CPython) 3.11.5And now I see that the version of black in the project requirements got bumped and I hadn't reinstalled the dependencies, so that explains it. |
Please open an issue (no obligation to take it on, but I agree this would be better so should document it.) |
There was a problem hiding this comment.
OK, I think this is pretty much good to go.
One thing would be if you could take a look over the docs and examples notebook to see if you feel anything needs updating as a result of these changes.
(It may well be that the answer is no as much of this is internals).
Possible things from a quick glance are:
- Target docstring - does it need the type updating to Literal/ScoringSystem for
system? - Rounds can now take iterable of passes, could have an example in the docstring for this, at least the tuple?
I added some general guidance on PRs here that includes this, but this work predates that, so don't worry about format for this.
My last query is about the apparent deprecation of TypeAlias in 3.12 (see comment) and whether we want to leave the code as is here (and remove the comment about bumping in v3.10+)?
Replied to the comment: in short I think it doesn't hurt to leave it since its unlikely to be there for long, and helps with discoverability. Will take a look through the docs, I'm not sure what best practice is for an aliased literal type like we have for scoring system. I've tried to find an example somewhere as to whether documentation mentions the actual base type for these, the aliased type or both.
This would be pretty long but would have the advantage of enumerating all the supported scoring systems right where a user might need to see them (in the docs for the Target class), disadvantage is it needs to be kept up to date with the code as new ones are added but that shouldn't be too often. However the style guide doesn't cover situations where a TypeAlias is used for a complex or lengthy (this case) type. Other options to keep it shorter include: Given the type is repeated in the docstring as both an init parameter and an attribute, I would lean towards choosing the explict list for the Parameters section (the target will literally refuse to initalise if you don't provide one of these strings), and then just describing the aliased and/or base type in the Attributes section (you can change it afterwards if you want, but mypy will shout at you) |
|
Figuting out how to get sphinx to play nicely with displaying the allowed values is defeating me here, so I'm not going to get too drawn into it. I've found from playing that if I Gives this: 
But clearly this is not much use in the actual docstring. I've left it as something that isn't quite as nicely formatted in the html docs, but fits with the suggested numpy docstring syntax, doesn't autofill and duplicate its values, and shows the relevant information in the docstring itself. |
Not caught by running black archeryutils
…no longer a dependency of, Sphinx.
…x and 'WoRKs oN mY mAcHInE'.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
==========================================
+ Coverage 95.45% 95.81% +0.35%
==========================================
Files 24 24
Lines 1321 1339 +18
==========================================
+ Hits 1261 1283 +22
+ Misses 60 56 -4
|
Closes #61
Should be compatible with python 3.9+ syntax. In order of increasing intensity:
The sum of the last three points allow for removing some of the extra assertions that were in place just to please mypy, especially in the score_for_round functions.
There are still a couple of Anys floating around that I might be able to excise, but this feels like most of it, and enough that I could do with a check over to see if anything else needs included in this one.
On the topic of making the scoring systems explicitly typed, I did think about looking at handicap systems as well, either as a literal type or an Enum, but with potential refactors coming from #2 that seemed like a step too far.
Other general feedback from the process of working on this: