-
Notifications
You must be signed in to change notification settings - Fork 2
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
ENH: expanded sim ioc generation, mfx example tree #68
base: master
Are you sure you want to change the base?
Conversation
… attr name from Value
3bdff50
to
f5d63a8
Compare
I'm intending to add a copy-the-ioc test and a pre-release docs before asking for reviews. |
tests go boom :( |
I should have caught that one, I forgot to re-run the tests after making "just one small change" |
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's a lot less to review here than I originally thought, though I'm mostly just assuming the autogenerated files are fine.
I don't really have much to criticize here. The new sim utilities look very useful and I look forward to using them even outside of BEAMS
I really appreciate all the comments / explanations, I think we'll be very grateful for them in the future.
|
||
The results from this should be fed into the test_ioc.py.j2 jinja2 template file. | ||
""" | ||
ctx = Context() |
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 too many contexts. I see this is not quite the same as pyepics' context, we just use it to gather PVs
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.
It's actually a close analog of the epics context in a lot of ways, just like pyepics's context (I think?) but the python apis are quite different between the two
print(exc) | ||
|
||
# This is probably overkill but it does save some time for large trees | ||
with Batch() as batch: |
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.
TIL this exists. This is a neat little alternative if a library is using caproto.
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.
Yeah, I decided to use caproto to measure since I was generating a caproto script anyway, so I assumed it would grant some consistency advantages. I'm not sure this assumption ended up being true but I learned a few tricks from the docs this time.
pv_info = default_pv_info(all_pvnames) | ||
else: | ||
pv_info = collect_pv_info(all_pvnames) | ||
sorted_pv_info = sorted(pv_info) |
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.
Does it matter that we sort the PVs? Or is it just for ease of reading through the rendered template
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.
Two reasons:
- In this implementation, the PVs are in essentially a random order (from the batch call) and I wanted a deterministic order to make regenerations of the same script consistent
- Easy of reading as you say
ENUM = pvproperty( | ||
value=2, | ||
dtype=ChannelType.ENUM, | ||
enum_strings=["e", "i", "pi"] |
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.
As far as defaults go I might pick something that doesn't collide with the STRING
default
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.
that's fair, I wanted all the values to be pi and took it too far
capsys: pytest.CaptureFixture, | ||
monkeypatch: pytest.MonkeyPatch, | ||
tmp_path: Path, | ||
): |
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.
This is the most python-y test I've seen in a while haha.
Description
Motivation and Context
How Has This Been Tested?
Interactively we can make it all the way through the tree!
I added and updated some unit tests.
Where Has This Been Documented?
Helpful docstrings and commments
Pre-release notes
Pre-merge checklist
docs/pre-release-notes.sh
and created a pre-release documentation page