jatkinson1000/archeryutils

Feature Request: Define Pass using existing Target

Closed this issue · 4 comments

Often we define a target before defining a Pass, especially in the examples.
However, we then pass exactly the same information to the Pass constructor that internally re-makes a Target.
It would be nice, if we have already constructed a Target, to pass this directly to an alternative Pass constructor.

May need to be careful with ensuring that we use a copy rather than a reference in the Pass...

import archeryutils as au

mytarget = au.Target("10_zone", (122, "cm"), (70.0, "m"))

mypass = au.Pass(36, mytarget)

I had been looking at this as part of #59 already as I needed to initalise Passes from an already constructed Target object there.
But I agree that it makes sense in general, and possibly should even be the default, as my thought process when working the REPL usually goes something like:

# make a target
>>> t = Target("10_zone", (122, "cm"), (70.0, "m"))

# try and make a pass with it
>>> p = Pass(36, t)
ValueError(...)

# swear, remember I have to recreate the Pass with the same parameters
>>> p = Pass(36, "10_zone", (122, "cm"), (70.0, "m"))

OK, My gut suggests doing it as a separate PR, but happy for it to come from #59 given you have already started.
I think having target as default, and then having a pass_from_base_target_data classmethod (not called that, but I can't come up with a more concise but clear name right now) is probably best, as you suggest.

Other option would be overloading, but I think that could get messy given they are mutually exclusive argument options rather than one being a subset.

I agree on a classmethod to replicate the current way of initalising passes makes sense, and consistent with the approaches to initialising Targets in that PR as well (seperate constructors rather than overloading). But not averse to factoring that bit out from there and doing it first to keep the changes more focused, especially given this is another breaking API change for all the tests.

. Naming it is tricky, I'd thought of: Pass.from_scratch, Pass.from_parameters, Pass.without_target. But I don't love any of them. And Pass.from_target_data is potentially confusing with the custom target concepts. How about Pass.and_target or Pass.with_target?
Maybe inspiration will come.

Got an implementation using at_target for the classmethod name, obviously that one open to change but it seems not too bad. Snuck in some useful reprs because trying to check these in the repl without them was deeply unpleasant.

Mini PR incoming.