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.

Sign up for GitHub

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

Jump to bottom

Scantool class and CI #2

Merged
merged 3 commits into from
Jun 21, 2023
Merged

Scantool class and CI #2

merged 3 commits into from
Jun 21, 2023

Conversation

JamesWrigley
Copy link
Member

This adds a component class for the scantool.

CC @IvarsKarpics

@JamesWrigley JamesWrigley self-assigned this May 30, 2023
@JamesWrigley JamesWrigley mentioned this pull request May 30, 2023
Closed
tmichela
tmichela approved these changes Jun 12, 2023
View reviewed changes
Copy link
Member

@tmichela tmichela left a comment

Choose a reason for hiding this comment

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

Codewise LGTM, though I haven't used the scantool, so I trust it does what it should.

Maybe I'd turn the attributes into properties, since you nicely documented what they are already, that'd make it easier to look them up from an interractive python session.

src/extra/components/scantool.py Outdated
from ..data import SourceData

class Scantool:
"""Interface for the XFEL scantool (Karabacon).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Interface for the XFEL scantool (Karabacon).
"""Interface for the EuXFEL scantool (Karabacon).

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to European XFEL in ba0e737.

@JamesWrigley
Copy link
Member Author

Thanks for the review 🙏 Possibly dumb question, but how would them being properties make them easier to look up interactively?

@tmichela
Copy link
Member

turn the comments in docstring?

@JamesWrigley
Copy link
Member Author

Ahhh right so you could call help() on them, makes sense.

@IvarsKarpics
Copy link

LGTM from my (scantool) side.

@JamesWrigley
Copy link
Member Author

Replaced attributes with properties in 7b93b0b (and fixed the tests).

@JamesWrigley
Copy link
Member Author

Rebased, and added CI in 747eaa1.

@JamesWrigley JamesWrigley changed the title Scantool class Scantool class and CI Jun 15, 2023
@JamesWrigley JamesWrigley force-pushed the scantool branch 3 times, most recently from 5b3116b to 460d717 Compare June 20, 2023 08:26
@JamesWrigley
Copy link
Member Author

Rebased onto docs: https://extra--2.org.readthedocs.build/en/2/

Base automatically changed from docs to master June 21, 2023 10:31
@JamesWrigley JamesWrigley merged commit cd22065 into master Jun 21, 2023
4 checks passed
@JamesWrigley JamesWrigley deleted the scantool branch June 21, 2023 10:45
@JamesWrigley JamesWrigley mentioned this pull request Jun 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmichela tmichela tmichela approved these changes

Assignees

@JamesWrigley JamesWrigley

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@JamesWrigley @tmichela @IvarsKarpics