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

Remove assert in production code #728

Merged
merged 102 commits into from
Jan 15, 2025

Conversation

kivel
Copy link
Contributor

@kivel kivel commented Jan 10, 2025

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 that bar 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.

kivel added 30 commits January 7, 2025 15:27
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
@kivel
Copy link
Contributor Author

kivel commented Jan 13, 2025

@coretl I addressed your requests to my abilities.

src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
@coretl
Copy link
Collaborator

coretl commented Jan 14, 2025

Please could you resolve the conflicts and remove that commented out code and then I will merge

@kivel
Copy link
Contributor Author

kivel commented Jan 14, 2025

Please could you resolve the conflicts and remove that commented out code and then I will merge
sure!

merged main into feature and fixed all conflicts 34e0b70

@coretl
Copy link
Collaborator

coretl commented Jan 14, 2025

Looks like it still has conflicts? Also, lint seems to be failing...

@kivel
Copy link
Contributor Author

kivel commented Jan 14, 2025

I have absolutely no idea why the ruff format step in the workflow insists on reformatting the 4 files and make it fail.
It looks to me as if a different formatting rule is used in the workflow. All the tools work on my host in the provide dev-container

(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

@coretl
Copy link
Collaborator

coretl commented Jan 15, 2025

I have absolutely no idea why the ruff format step in the workflow insists on reformatting the 4 files and make it fail.

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 pip install ruff --upgrade in your venv then ruff format then it should match CI

@kivel
Copy link
Contributor Author

kivel commented Jan 15, 2025

@burkeds Thanks for looking into the Tango part.

@coretl Thanks for pointing out the need to update ruff. That made the CI work.
That should conclude this PR.

@coretl coretl merged commit a59ddcf into bluesky:main Jan 15, 2025
14 of 15 checks passed
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.

3 participants