jatkinson1000/archeryutils

Units for target face diameter

jatkinson1000 opened this issue · 3 comments

Discussed in #17

  • Add option for user to specify target face diameter unit
  • Set default to be [cm]
  • Store under the hood as [m] to preserve SI and not affect code elsewhere

This requires:

  • Add optional argument to Target class
  • Add conversion code as part of __init__ in Target class
  • Add INCHES2M and CM2M to constants file
  • Add tests for conversion process
  • Update and round data files accordingly

Initial change to the Target constructor is easy enough, have that drafted up.
But the question that arises is what do you want to do about Pass? At the moment it contstructs a Target from the raw parameters passed to it, so changing the default target diameter unit to cm has broken many, many things.

The simplest change is to just assume the units of the value passed to Pass are cm instead of m.
The other option is to replicate the api, give Pass a deafult diameter unit of cm as well and let it pass through to the Target constructor for conversion

Looked at it a bit more, was actually very easy to update all the tests to cm instead of m for diameters, and load_rounds just needed a single change to remove the division by 100. Can upload a pull request for this now

Yep, I just had a look and I think passing a "face size unit" to the pass constructor that goes through to the target constructor is the way to go.
Please do open a PR and I will do a review.
We can move this discussion over there (my development preference).