Add calculate_age_group#134
Conversation
|
The failing test on python 3.10 relates to the use of |
There was a problem hiding this comment.
Thanks for this @mjtamlyn I think it's a really useful utility
Did some slight restructuring and added null to the json to satisfy Python 3.10 with all ages having a max/min which could be None. Also relocated the tests to follow the current structure.
Hopefully this all still works for the situations in which you envisage using this in your code, but let me know if not!
My only comment would be whether it is worth renaming the function to calculate_agb_age_group()? This would match the call signs for the agb classifications and leave open to implementing other schemes in future.
Edit: I'm currently implementing Archery Australia classifications - I wonder if passing an optional argument for rules (like with handicaps) might be useful. e.g. "AGB" vs "AA".
|
Thanks for the updates to make it work with the strict typing. I'm happy for now to look at just renaming to
Even if we factor this out in the long term, it's reasonable to expose So I reckon just rename the fn and we're good to go? |
This utility function works out the age group under AGB rules, given a year of birth and the year the event takes place in.
…ure sorting explicitly by age value.
The return statements make use of AGB_ages to provide values. Allowing other options as an input leaves open to error.
a39ec38 to
eef1291
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #134 +/- ##
==========================================
+ Coverage 98.28% 98.31% +0.03%
==========================================
Files 34 36 +2
Lines 2326 2380 +54
==========================================
+ Hits 2286 2340 +54
Misses 40 40
🚀 New features to boost your workflow:
|
|
@mjtamlyn I just rebased, updated to use the new ages enum, and renamed the function. I believe this is good to go if you can take a quick look and are happy. My final thought is whether this should be in the classifications module or a new module ( Let me know your thoughts and approval and then I'll look to merge. |
|
PR in its current form looks great. I'm indifferent about its location. |
61111ce to
81bcce5
Compare
This utility function works out the age group under AGB rules, given a year of birth and the year the event takes place in.
I've written this (or something like it) more than once, and it seems like a "core" utility function for AGB applications, usable in records code, entry systems, rankings etc.
I think I've done most of the docstrings, testing, type checks etc in roughly the right way, but let me know if htere's things missing.