-
Notifications
You must be signed in to change notification settings - Fork 28
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
Remove assert in production code #728
Remove assert in production code #728
Conversation
strict check for ZERO velocity, sim Mover and epics Motor
minor refactor to reduce nesting in more linear flow with `elif`
- added TypeGaurd to make Pylance happy. - added TypeError raised test
Co-authored-by: Tom C (DLS) <[email protected]>
@coretl I addressed your requests to my abilities. |
Please could you resolve the conflicts and remove that commented out code and then I will merge |
merged main into feature and fixed all conflicts 34e0b70 |
Looks like it still has conflicts? Also, lint seems to be failing... |
I have absolutely no idea why the (venv) root ➜ /workspaces/ophyd-async (710_remove_assert) $ ruff format
178 files left unchanged
(venv) root ➜ /workspaces/ophyd-async (710_remove_assert) $ pre-commit run -a
check for added large files..............................................Passed
check yaml...............................................................Passed
check for merge conflicts................................................Passed
fix end of files.........................................................Passed
lint with ruff...........................................................Passed
format with ruff.........................................................Passed
Ensure import directionality.............................................Passed
(venv) root ➜ /workspaces/ophyd-async (710_remove_assert) $ git status
On branch 710_remove_assert
Your branch is up to date with 'origin/710_remove_assert'.
nothing to commit, working tree clean |
I think that is because ruff changed its formatting rules (looks like it does that once a year): https://github.com/astral-sh/ruff/releases/tag/0.9.0 If you run |
The changes will remove
assert
statements and replace them with check/raise.Logic should remain identical, except for velocity related assertions in the timeout calculation.
new ruff rule
rule S101 was added to avoid future use of
assert
Tango
There are a few remaining uses of assert in the Tango code. I ask for a Tango developer to take a look at those. If fail understand why they are used and also struggle with seemingly unrelated tests failing when I touch them.
assert in general
I understand that a simple
assert foo.bar
will make language support tools happy, as it is certain thatbar
exists for subsequent code. There are alternatives, admittedly, not as straight forward, though.The general question for code hygiene should be, what comes first, a happy type checker or readable code. It happened a few time in my quest to remove the assert statements that it was unclear to me, why assert was used in the first place. I'd at least add a comment, if assert is used to make Pylance happy.
velocity checks
Previously, the velocity needed to be positive. The error message was stating however that the velocity is zero. Since the EPICS motorRecord will work with both, the check was changed to specifically check for zero. Technically, the check isn't even needed, as the subsequent timeout calculation will throw a DivisionByZero exception. This is of course, in the case we allow negative velocities ... which we should.