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

Don't use assert outside of tests #1533

Closed
fabianegli opened this issue Apr 26, 2022 · 4 comments
Closed

Don't use assert outside of tests #1533

fabianegli opened this issue Apr 26, 2022 · 4 comments
Labels
command line tools Anything to do with the cli interfaces low-priority

Comments

@fabianegli
Copy link
Contributor

Description of feature

The problem with assert is that it is not executed if python is invoked with optimizations. This means that the code behaves differently and this is probably not desired.

Use git grep assert nf_core to find where the problem lies.

See this stackoverflow answer for explanations and also this biopython issues for an example of a codebase that followed the suggestion and more info.

@ewels
Copy link
Member

ewels commented May 5, 2022

Yeah I read this relatively recently and realised the same, that we're using these functionally all over the place.

It's a hassle to update though, as we do escalation of exceptions as a fairly common pattern. So replacing with if statements often won't work for us, we'll need to replace with a different exception type. And then we'll need to track back up the entire call stack to see where those exceptions are handled, and deal with them there.

I have thought that it could be nice to create a file with a set of dedicated nf-core specific exceptions that we use. This would be nice for any packages that want to use nf-core code as they could then handle them specifically. But again, a fair amount of code for very little gain in the practical sense, so has been in my "low priority" box for a while.

@ewels ewels added low-priority command line tools Anything to do with the cli interfaces labels May 5, 2022
@ewels ewels changed the title [ENH] Use if instead of assert outside of tests Don't use assert outside of tests May 5, 2022
@fabianegli
Copy link
Contributor Author

if combined with raise AssertionError(...) retains the behaviour and makes the control flow optimization resistant if an AssertionError is desired, but I expect that in some cases a different exception like a UserWarning, ValueError or TypeError will be more appropriate.

@ewels
Copy link
Member

ewels commented Jun 7, 2022

Yeah, or writing some custom exception types like I did for the sync code here. That's probably my favourite approach. Especially if we make some general ones which other specific ones can be based on - then we can catch the general ones up at CLI level easily.

Another example of a project I was involved in where we did this more extensively is here: https://github.com/ScilifelabDataCentre/dds_cli/blob/dev/dds_cli/exceptions.py

@fabianegli
Copy link
Contributor Author

Closed by #1685

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command line tools Anything to do with the cli interfaces low-priority
Projects
None yet
Development

No branches or pull requests

2 participants