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

Linting process names with multiple underlines is broken #2205

Closed
fabianegli opened this issue Mar 19, 2023 · 2 comments
Closed

Linting process names with multiple underlines is broken #2205

fabianegli opened this issue Mar 19, 2023 · 2 comments

Comments

@fabianegli
Copy link
Contributor

fabianegli commented Mar 19, 2023

When linting the quantms pipeline tools complains about a non-standard process label process_very but the label is actually process_very_low.

Is there a naming convention which says a label can only use one underscore? I guess tools somewhere splits the string and then only uses the two first segments. Or has some other way to truncate labels. Either way this need asks for a fix.

@awgymer
Copy link
Contributor

awgymer commented Mar 29, 2023

try:
    process_label = re.search("process_[A-Za-z]+", process_label[0]).group(0)
except AttributeError:
    process_label = re.search("'([A-Za-z_-]+)'", process_label[0]).group(0)

I don't know if the first regex is just an oversight but it clearly wasn't expected to have one.
I suppose the assumption was that if you are using process_ you would be using (or trying to use) one of the standard labels, which all have just one underscore.

The fact that a non-standard label breaks isn't really an issue imo the real problem here is that process_low_nonsense would get matched as process_low and there'd be no warning!

@awgymer
Copy link
Contributor

awgymer commented Mar 29, 2023

Looking at the code for this lint check I am wondering whether it needs more fixes. It currently makes several assumptions (e.g. if there is more than 1 label it only check the first one found) and doesn't necessarily perform much sense-checking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants