Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
==========================================
- Coverage 91.84% 91.74% -0.10%
==========================================
Files 40 40
Lines 1961 1962 +1
==========================================
- Hits 1801 1800 -1
- Misses 160 162 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This new function should replace the one in fastcs.util and it should not be private. That function is (rightly or wrongly) used by fastcs-odin for converting names. If this new method does not succeed in converting to PascalCase it will still be possible to get pvi validation errors, but that is probably OK. Could you try to provoke this in a test? It would be good to have a unit test for the function parameterized with various combinations that it handles and does not handle. |
|
Sure! I can do those changes. |
LuisFSegalla
force-pushed
the
pv_creation_helper
branch
from
af8ec74 to
1ac5fa4
Compare
|
Sorry I meant we want the function that is currently in pva, because I think that is a better implementation, but we want it to replace the one in fastcs.util. Thanks that test is useful. I am thinking that this function should just do exactly what it says it does and not even handle dashes instead of underscores. We could do a second pass where we try to make this more generally useful. Something else we could do is enforce snake case when adding to Controller.attributes, as the context of the error will be in the driver code and be much more useful than when calling the pvi function. |
|
Oh my bad!! |
|
I guess I was thinking what pydantic does when validating a serialised form, but we can't do that here so would need to do something else. Maybe a method for adding to |
LuisFSegalla
force-pushed
the
pv_creation_helper
branch
from
37d9b0b to
f1373e7
Compare
LuisFSegalla
force-pushed
the
pv_creation_helper
branch
from
f1373e7 to
861d4b9
Compare
|
I am not sure why the existing function has 3 steps. I think it should just be re.sub(r"(?:^|_)([a-z])", lambda m: m.group(1).upper(), s)This doesn't assert the full match, so we get "name_with-different_separators" -> "NameWith-differentSeparators", which is a bit odd, so it should also return the original if it is not snake case. if not re.fullmatch(r"[a-z]+(?:_[a-z]+)*", s):
return sThis may be a bit limiting, but at least it is clear what it is doing. It could be extended in future. |
|
That makes sense! |
|
Yes good point. It should allow digits too (just not for the first character). |
|
OK I think this ready now, but I would like to try it with PandA, Eiger and Odin to make sure it doesn't break things. @LuisFSegalla have you been testing this branch againts fastcs-odin? @jsouter would you be able to try this branch with PandA and Eiger to see if it looks OK? |
|
I tested agianst fastcs-odin (main branch) and fastcs-xspress (update-adapter) and nothing broke! |
|
fastcs-PandABlocks seems to run okay against this, and the tests that are there pass. This is the output for the top level PVI. and for one of the blocks but then some blocks are still snake cased |
|
@shihab-dls could you test fastcs-eiger against this branch and try the ophyd device against it? |
Updated public snake_to_pascal function and added tests for it.
Reverted to using regex logic on snake_to_pascal function and only handle "_" character
Made function only convert strings that are valid snake case and removed redundant code parts.
GDYendell
force-pushed
the
pv_creation_helper
branch
from
203aea8 to
d17abbb
Compare
I managed to run a brief dodal plan using the fastcs-eiger ophyd-async device against this PR (although with some unrelated hiccups on the odin side); tests also pass and PV names look good to me! |
4 participants
Added a single helper function to convert PV names into Pascal case and replace both "-" and "_" characters.
I'm opening the PR to discuss if more actions might be necessary for the helper as it's currently not checking for any other characters (like "/", "%". ...) and it required a small change in the tests that I'm not entirely sure is correct.
Closes #42