snakemake / snakefmt

The uncompromising Snakemake code formatter
MIT License
147 stars 28 forks source link

[Code curiosity] Exceptions #177

Closed chillenzer closed 1 year ago

chillenzer commented 1 year ago

Hi, first of all thanks for the great work! It's great to have such a beautiful infrastructure around snakemake.

Recently, I've been hunting down a bug (in either my use or snakefmt itself -- might become a separate issue later) and was therefore strolling through the code. I found the raise SyntaxError in exceptions.py where it was raised inside of a function NotAnIdentifierError via raise SyntaxError(...). Then, I looked up where that function was called and the call site looked like

raise NotAnIdentifierError(...)

I blinked once or twice and then went back to the exceptions.py to check whether NotAnIdentifierError could be raised (i.e. was a subclass of Exception). Turns out it wasn't and, in fact, it doesn't return anything. So, my understanding is that the above line tries to raise a None (which would fail leading to another Exception) but never gets to that point because during the evaluation of NotAnIdentifierError an Exception is raised.

Practically, that does what is intended really, so this is not precisely a bug report. But it's still a bit peculiar and I wondered (a) if you are aware of this (some other errors are subclassing Exception, so I guess you are), (b) if I missed something and this is actually a valid way of subclassing Exception, and (c) if you might want to change that.

Without looking that up and/or testing it, I suppose the correct way to achieve what you intended would be

class NotAnIdentifierError(SyntaxError):
    def __init__(self, line_nb: str, identifier: str, keyword_line: str):
        super().__init__(f"{line_nb}'{identifier}' in '{keyword_line}' is not a valid identifier")

although to be 100% precise, that would change the error message from starting with SyntaxError to starting with NotAnIdentifierError.

Best, Julian

mbhall88 commented 1 year ago

Hi Julian,

Thanks for the report. This is indeed a bug. Thanks for reporting. I'll get this fixed soon. And let us know if we can help to figure out your other bug you're hunting.

chillenzer commented 1 year ago

Happy to help! =)