Improve process label linting

[awgymer]

Arising from #2205

Current behaviour is to look for label declarations but only lint the first one occuring.
This PR aims to lint each label and check for any conflicts and the presence of 1 standard label.

Non-standard labels are currently not warned about.

PR checklist

• This comment contains a description of changes (with reason)
• CHANGELOG.md is updated
• If you've fixed a bug or added code that should be tested, add tests!
• Documentation in docs is updated

[Aratz]

Nice work! I left a few comments. You should probably add a test based on the scenario described in #2205 as well.

[fabianegli]

It might be a good idea to extract this into a function. Something like get_process_label. That would make things more readable and testable. And also have tests.

[awgymer]

The whole method is now getting quite bloated. If the code here is accepted then I think a next step could be to split out the whole label section rather than just splitting out the list comprehension. Similarly it may be worth considering splitting out the container parsing part.

[fabianegli]

That was then less clear than I hoped. I meant taking out the whole label selection as you propose - not just a list comprehension.

[fabianegli]

And I think it would be ideal to do it as part of this PR.

[codecov]

Codecov Report

Merging #2227 (18b6d1a) into dev (df60f0c) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #2227      +/-   ##
==========================================
+ Coverage   73.09%   73.19%   +0.09%     
==========================================
  Files          77       77              
  Lines        8408     8423      +15     
==========================================
+ Hits         6146     6165      +19     
+ Misses       2262     2258       -4     

Impacted Files	Coverage Δ
nf_core/modules/lint/main_nf.py	67.88% <100.00%> (+3.07%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[mirpedrol]

LGTM! but agree with @fabianegli's comment, it would be good to put this code in a separate function check_labels() for example

[Aratz]

I still think it would be good to add at least one test based on #2205 and I agree with @fabianegli that it would be good to extract this part of the code into it's own function (would make testing easier too!).

[awgymer]

@mirpedrol I am going to try and write test(s) now but just wondering whether some of the warnings need to be "new" tests?

For example right now all these warnings go under process_standard_label (I've just extracted the parts with the warnings).

self.warned.append(
                    (
                        "process_standard_label",
                        f"Specified label appears to contain non-alphanumerics: {label}",
                        self.main_nf,
                    )
                )
                
 if len(good_labels) > 1:
            self.warned.append(
                (
                    "process_standard_label",
                    f"Conflicting process labels found: `{'`,`'.join(good_labels)}`",
                    self.main_nf,
                )
            )
        elif len(good_labels) == 1:
            self.passed.append(("process_standard_label", "Correct process label", self.main_nf))
        else:
            self.warned.append(("process_standard_label", "Standard process label not found", self.main_nf))
        if len(bad_labels) > 0:
            self.warned.append(
                ("process_standard_label", f"Non-standard labels found: `{'`,`'.join(good_labels)}`", self.main_nf)
            )
        if len(all_labels) > len(set(all_labels)):
            self.warned.append(
                (
                    "process_standard_label",
                    f"Duplicate labels found: `{'`,`'.join(sorted(all_labels))}`",
                    self.main_nf,
                )
            )
    else:
        self.warned.append(("process_standard_label", "Process label not specified", self.

I'm not sure whether non-standard process labels found is something that will break CI if people use them but also use the flag that makes all warnings failures?

[mirpedrol]

I would say it's the expected behaviour to have get_biocontainer_tag as warnings and failing if someone uses --fail-warned 🤔

[awgymer]

Test added in a kind of unique way as no previous tests check for individual failure/pass cases but this should!

[Aratz]

Thanks for taking the time to add the tests, I only have one comment

[mirpedrol]

I think the test is not executed, should be added to test_modules.py, but I'm not sure how to do this when using the @pytest.mark.parametrize decorator.

[awgymer]

So it turns out you were right but also because we use unittest.TestCase I cannot parametrize that function. I've just added the test cases as a loop in the test for now. I think it's OK like this for now (to make the release) but we might want to refactor that in future.

[mirpedrol]

LGTM like this 🙂
#2226 could be the way to refactor this in the future

Changes made.