Make ControllerVector generic on Controller_T#326
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #326 +/- ##
==========================================
+ Coverage 90.85% 90.86% +0.01%
==========================================
Files 70 70
Lines 2547 2550 +3
==========================================
+ Hits 2314 2317 +3
Misses 233 233 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| from fastcs.controllers import BaseController # noqa: F401 | ||
|
|
||
| Controller_T = TypeVar("Controller_T", bound="BaseController") # noqa: F821 | ||
| """Generic `Controller` class that an unbound method must be called with as `self`""" |
There was a problem hiding this comment.
Is there any reason not to define this in base_controller.py?
There was a problem hiding this comment.
Since it's used in methods and controllers, I thought util was most sensible, but I guess methods importing from controllers is not that strange; is this a nit?
There was a problem hiding this comment.
Not even a nit. I am not sure what is best.
util.py is kind of intended to be a place that everything can import from. If it itself needs to import from the modules then that breaks. You couldn't import util from the controllers module now because it won't be fully initialised, although because of the if TYPE_CHECKING it is OK
I think it makes sense to put it in controllers somewhere. It could even be defined in __init__.py.
Would you prefer where it is currently?
There was a problem hiding this comment.
Hmm, I agree with you, utils importing from a sub-directory is not ideal; I liked the idea of putting Controller_T in controllers __init__, but this makes it partially resolved for methods, which forces it into the if TYPE_CHECKING condition, which is no good, as we need it to exist at runtime. Keeping it in utils, although not an intended use of a utils file, lets us grab Controller_T in both methods and controllers at runtime, and type check Controller_T during linting due to the IF TYPE_CHECKING condition on BaseController. Although, this would break if we started enforcing import contracts in the future... What do you think?
2 participants
closes #325