-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove public fields of BlueskyContext and replace with methods #174
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #174 +/- ##
==========================================
+ Coverage 83.00% 83.06% +0.05%
==========================================
Files 39 39
Lines 1024 1045 +21
==========================================
+ Hits 850 868 +18
- Misses 174 177 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few docs changes here not related to replacing fields with methods. Is it picky to suggest these are committed/merged as separate commits when the PR is merged?
def find_plan_metadata(self, name: str) -> Optional[Plan]: | ||
return self._plans.get(name) | ||
|
||
def all_devices(self) -> Iterable[Device]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth making these two properties as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or not having any properties... I'm always on the fence between the potential for confusion and the nice read-only-ness
func = ctx.plan_functions[self.name] | ||
sanitized_params = _lookup_params(ctx, plan, self.params) | ||
plan_generator = func(**sanitized_params.dict()) | ||
metadata = ctx.find_plan_metadata(self.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these two functions are only ever called together and both have to be present, it'd be clearer to have a single find_plan(name) -> Optional[(Plan, PlanFunc)]
. Probably suggests plans
and plan_functions
should be merged into a single field in the context as well. Maybe a dict of name -> (Plan, func)
?
It seems more sustainable to use private fields and access them via methods. The device and plan dictionaries and run engine are now hidden behind methods.