Fix S101 Use of assert detected
#1783
Conversation
| settings.DANDI_DANDISETS_LOG_BUCKET_NAME, | ||
| settings.DANDI_DANDISETS_EMBARGO_LOG_BUCKET_NAME, | ||
| ] | ||
| }: | ||
| raise ValueError(f'Non-log bucket: {bucket}') |
There was a problem hiding this comment.
Why did you choose a ValueError here, but RuntimeError for the same error below?
Also, why the change from a list to a set?
There was a problem hiding this comment.
I understand that the order of the buckets is irrelevant, which I'm guessing is why it was converted, but I've come to really dislike the braces-style set syntax. I hardly see it used in the wild and it doesn't work for empty sets, which just makes it confusing IMO.
There was a problem hiding this comment.
For those cases, since the problem was directly caused by the given value of a parameter, ValueError seemed like a better fit:
Raised when an operation or function receives an argument that has the right type but an inappropriate value, and the situation is not described by a more precise exception such as IndexError.
The more generic exception is RuntimeError:
Raised when an error is detected that doesn’t fall in any of the other categories. The associated value is a string indicating what precisely went wrong.
There was a problem hiding this comment.
I made it a set because that's a more appropriate collection for things which have no ordering and where membership testing is a primary consideration. Certainly here, it makes no real difference either way.
|
🚀 PR was released in |
In non-test parts of the codebase,
assertshouldn't be used. See https://docs.astral.sh/ruff/rules/assert/ for rationale.