Skip to content

Add Handler.initialise(controller) #135

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

Merged
merged 17 commits into from
May 19, 2025
Merged

Conversation

ajgdls
Copy link
Contributor

@ajgdls ajgdls commented Mar 12, 2025

Resolves #59
Added default initialise methods to Sender and Updater base classes.
Added initialise method to Attribute, checks for handler and calls the method.
Moved initialise into BaseController class, loops over attributes calling initialise with a reference to self.
Added unit test for Attribute class.

Fixes #59
Fixes #130

@ajgdls ajgdls requested a review from GDYendell March 12, 2025 15:00
Copy link

codecov bot commented Mar 12, 2025

Codecov Report

Attention: Patch coverage is 94.23077% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.81%. Comparing base (6a8c649) to head (d52f300).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/fastcs/controller.py 84.61% 2 Missing ⚠️
src/fastcs/backend.py 94.44% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #135      +/-   ##
==========================================
- Coverage   90.83%   90.81%   -0.03%     
==========================================
  Files          41       41              
  Lines        1954     1971      +17     
==========================================
+ Hits         1775     1790      +15     
- Misses        179      181       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jsouter
Copy link
Contributor

jsouter commented Mar 13, 2025

Could be helpful for setting up things like converters so we can have less logic inside the update/put calls!

Copy link
Contributor

@evvaaaa evvaaaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think as part of this change we should take out controller as an argument to put/update, making this PR unneccesary.

If the user wants the controller in the handler for use in put or update then they should store it as an attribute (in the python sense) in their handler from initialise.

We can take out controller as an argument here

def _link_attribute_sender_class(
controller_api: ControllerAPI, controller: Controller
) -> None:
for attr_name, attribute in controller_api.attributes.items():
match attribute:
case AttrW(sender=Sender()):
assert not attribute.has_process_callback(), (
f"Cannot assign both put method and Sender object to {attr_name}"
)
callback = _create_sender_callback(attribute, controller)
attribute.add_process_callback(callback)

and here

def _create_updater_callback(attribute, controller):
async def callback():
try:
await attribute.updater.update(controller, attribute)
except Exception as e:
print(
f"Update loop in {attribute.updater} stopped:\n"
f"{e.__class__.__name__}: {e}"
)
raise
return callback

@GDYendell

@ajgdls ajgdls force-pushed the 59-add-handler-initialise branch from 66d4dc7 to 8005664 Compare March 26, 2025 11:25
@ajgdls ajgdls force-pushed the 59-add-handler-initialise branch from 6da6ec6 to e7430a5 Compare May 15, 2025 16:10
@ajgdls ajgdls marked this pull request as ready for review May 15, 2025 16:53
@GDYendell
Copy link
Contributor

GDYendell commented May 16, 2025

This is actually broken and is showing up a lack of testing. The updater callbacks are still being called with a controller here, which causes them to stop and print a warning.

The reason this exception is caught and printed was originally so that one attribute update failing doesn't crash the whole application. I was on the fence about this and am again thinking just crashing would be better.

Although to be fair if this had type hints that would have caught it.

@ajgdls
Copy link
Contributor Author

ajgdls commented May 16, 2025

Good spot, I guess you noticed this by running an application? Perhaps I should add the type hints as this is not really a runtime exception but a develop time syntax error.

@GDYendell GDYendell force-pushed the 59-add-handler-initialise branch from 2921205 to d52f300 Compare May 19, 2025 13:51
@GDYendell GDYendell merged commit 5495fde into main May 19, 2025
18 checks passed
@GDYendell GDYendell deleted the 59-add-handler-initialise branch May 19, 2025 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sender/Updater/Handler now incorrectly take the root controller Add Handler.initialise(controller) and call during Backend initialisation
4 participants