Skip to content
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

Update to use new attribute API and pyright #57

Merged
merged 4 commits into from
Dec 19, 2024
Merged

Update to use new attribute API and pyright #57

merged 4 commits into from
Dec 19, 2024

Conversation

jsouter
Copy link
Collaborator

@jsouter jsouter commented Dec 6, 2024

Closing #50

@jsouter jsouter requested a review from GDYendell December 6, 2024 14:25
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 73.07692% with 7 lines in your changes missing coverage. Please review.

Project coverage is 83.94%. Comparing base (3bdfc71) to head (1e4e869).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/fastcs_eiger/__main__.py 28.57% 5 Missing ⚠️
src/fastcs_eiger/eiger_controller.py 89.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #57      +/-   ##
==========================================
- Coverage   84.06%   83.94%   -0.13%     
==========================================
  Files           4        4              
  Lines         295      299       +4     
==========================================
+ Hits          248      251       +3     
- Misses         47       48       +1     

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

@jsouter jsouter marked this pull request as draft December 6, 2024 14:27
@jsouter jsouter marked this pull request as ready for review December 9, 2024 14:45
pyproject.toml Outdated Show resolved Hide resolved
src/fastcs_eiger/eiger_controller.py Show resolved Hide resolved
src/fastcs_eiger/eiger_controller.py Outdated Show resolved Hide resolved
tests/system/test_introspection.py Show resolved Hide resolved
tests/system/test_introspection.py Outdated Show resolved Hide resolved
@jsouter
Copy link
Collaborator Author

jsouter commented Dec 10, 2024

match getattr(self, attr_name, None):

I'll need to fix this too

@jsouter jsouter merged commit ffd383c into main Dec 19, 2024
17 of 19 checks passed
@jsouter jsouter deleted the pyright branch December 19, 2024 11:45
"""
Handler for FastCS Attribute Creation

Dataclass that is called using the AttrR, AttrRW function.
Used for dynamically created attributes that are added for additional logic
"""

async def put(self, _: "EigerController", attr: AttrW, value: Any) -> None:
async def put(self, controller: BaseController, attr: AttrW, value: Any) -> None:
assert isinstance(attr, AttrR) # AttrW does not implement set
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary if attr is AttrW?

Copy link
Collaborator Author

@jsouter jsouter Dec 19, 2024

Choose a reason for hiding this comment

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

AttrW doesn't have set so pyright complains if we're trying to call set if we don't verify it's an AttrR (or I guess AttrRW).

if isinstance(value, list) and all(
isinstance(s, str) for s in value
): # error is a list of strings
value = ", ".join(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting it like this might make it look like two messages is actually one, so it is probably clearer to just do str(value) Or maybe use a different separator, like |?

I do wonder if this should be implemented in FastCS with String().validate(value).

Copy link
Collaborator Author

@jsouter jsouter Dec 19, 2024

Choose a reason for hiding this comment

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

I have a feeling that in a previous release lists did automatically get cast to strings, though I may be misremembering and that was just in a dev branch I never pushed anywhere.

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.

2 participants