Improve input to classification functions

[jatkinson1000]

At present inputs to the classification functions are clunky and fickle, as per a couple of points raised by @TomHall2020 in #103

• Lots of string inputs
• Unclear what is expected 'ladies', 'female', 'women' etc.
• Hard to get information on what is expected - not in docs, no useful feedback when wrong, have to examine source.

It would be nice to resolve some of these.
I have been pondering this for a while and will consolidate thoughts here before deciding and tackling the issue.

• Have a 'category' class
  • input are strings, but returns a definitive format that is then used as single input to classification functions
  • variation in inputs (upper/lower cases etc.) handled here in one definitive piece of code
  • But how to handle different systems - e.g. old and new age groups?
  • Will break the current API
  • Feels clunky - need to construct this class before calling a classification function, and checks still need to be done
• Use a python Enum
  • Provide an enum of form AGB_outdoor_ages.U18
  • Perhaps using a Flag class of enum we could have all categories listed, but then create subsets as relevant to the different schemes
  • Use class methods from the enum flag to check things
  • Make enum flag available at classification import level - this would provide users with feedback about acceptable inputs at runtime.
  • Could access the string and number part from my_age = AGB_outdoor_ages.U18 using my_age.name and my_age.value for purposes of looking up in classification dicts
  • Will break API
  • Number of inputs to the classification functions stay the same, but now rigorously restricted and options can be documented/queried.

Other thoughts:

• This will likely be API breaking, which means we'd need to move to version 2.0
  • Irritating, as prefer to avoid this, but userbase is probably still localised enough (I think...) and this issue serves as first advance warning.
• Moving to U18, U16 would be nicer than Under 18
  • But what to do for "Adult"? SNR? - could be confused with 50+. A? - bit short.
• could the above suggestions somehow be combined?
  • E.g. option of specifiying age, bowstyle, gender separately to function, or as one "class" item?
    • Probably not worth it, and creates overly verbose code.
• Similarly, is there a way to make this backwards compatible
  • Not fully I think, and again, probably not worth it creating overly verbose spaghetti code

[jatkinson1000]

Further thoughts whilst looking at this:
At present we can take an innappropriate bowstyle to a classification scheme and return the appropriate classification.
e.g. compound barebow becomes compound in target, and flatbow becomes barebow.
Using the enum approach we could restrict this to enforce only appropriate bowstyles for that scheme, but that might cause issues elsewhere - e.g. if an application sets a user's bowstyle just once for all schemes.

Conversely one might argue that this code should adhere strictly to the letter of the rulebook, with any "workarounds" handled at a higher level by a particular application.

I thought I preferred the first option, but typing this I am perhaps leaning towards the latter...

[jatkinson1000]

Enum values must start with a char and cannot contain operators.

So 50+ is not an option which is a pain.
Consider P50, or PLUS50? I don't really like either of these.
OVER50 or O50 matches U15 etc. but I don't really like this either.

Perhaps this becomes a strict dict rather than an enum?
Is enum really the best approach here...?

[jatkinson1000]

Handicaps take a Round object.
Classifications probably should as well, rather than a string, for consistency.
Could then check it is appropriate for the scheme and return a meaningful error rather than just falling over if not a key in the appropriate rounds dict.

[TomHall2020]

Have done some preliminary looking at a lot of this, will edit here to put up some info on it later when I get the chance. I did try implementing a Category dataclass that contained enums for each of bowstyle, age, gender, and also allowed passing of string values for those who really just want to footgun themselves (still better as the errors raised would reveal the allowed options via the enum), the result wasn't bad for verbosity (could just wrap the three parameters in existing function calls with the dataclass contrsuctor).

I have absolutely no idea how to implement it but I'd love a fluent Enum on the lines of Category.RECURVE.U15.FEMALE that was insentive to order and gave all the completions. Similar to how it works in this library redesign talk, which I found quite useful in general, but their implementation is quite nasty and relies on code-generating new dataclasses for every possible combination of completions.

One real conceptual issue is: do the age classes belong to the classification scheme itself AGB.U18, AA.RECURVE or to a generic concept Age.O50, Bowstyle.COMPOUND? Limited implications for now because of the alignment between AGB and world archery categories, and I don't think AA doesn anything drastically different.

[jatkinson1000]

Thanks, look forward to seeing it.

I also had a bit of a play this afternoon and may upload once minimally working.
In answer to a couple of your points about what 'belongs' to a classification my approach was to have a generic AGB_ages Flag enum containing all possible AGB ages.
Within the classification scheme I defined a subset AGB_ages_field, say, using the | operator.
This meant that the inputs could now come from the generic AGB_ages exposed to the top-level user, but checked to ensure they were appropriate with a warning raised if the input wasn't part of the AGB_ages_field subset.
Similar for AGB_bowstyles.
I felt this was a balance between simplicity for user and codebase.

As something of a halfway house you see that I defined AGB_ages to cover all age categories that AGB uses, with anticipation that should I implement the AA classification scheme there would be AA_ages that gets used in those files/routines.
I suppose one asks then if there should be a dict of enums allowing archeryutils_ages.AGB_ages.U18 etm...

I'm still not super happy with what to name the categories...
O50 falls under the cardinal sin of mixing O and 0, and I don't feel is super intuitive.

That talk looks really interesting, thanks for the heads up.
Though wondering if I can make watching it 'work' after the 6th ;)

Since it has been a while since I used the library in anger I have found it very enlightening to come back to the examples with a fresh view of "how do I intuitively feel like I want to use this".
E.g. I agree with your point elsewhere that classifications should take a Round, not a string (checked internally for suitability).

[jatkinson1000]

@TomHall2020 You can see the start towards this using enums in #113

Still a few unresolved issues regarding the philosophy of "what belongs where" and naming, plus version updates.

[jatkinson1000]

The enums are now ready to go in #113 and the ability to pass Rounds to classification functions is handled in #119

The combination of the two of these should resolve this.

Now need to reflect on the new API and decide if it is the best option available within the compromises before merging for version 2.0.0

Any thoughts you might have much appreciated @TomHall2020 if you have chance to look at these.

[TomHall2020]

Yeah sorry for abandoning you to this for a while! Had a quick poke at #119, need to go off and actually shoot a round right now, will review #113 tonight but broadly definitely looks like its a step in the right direction.

[jatkinson1000]

#119 and #113 closed this.
Will be released as part of version 2.0.0.