Increase worflow linting #1463

lldelisle · 2024-06-29T10:32:33Z

I am not sure it is the good way to do it.
For the moment, I cannot make it like for the tools with classes, in order to make it customizable but I think I put everything I thought was missing in the current workflow_lint.
I used sometimes ' and sometimes ". Is this an issue?

lldelisle · 2024-06-29T10:41:39Z

I don't know how to fix:

./planemo/workflow_lint.py:434:1: C901 '_lint_dockstore_workflow_entry' is too complex (15)

lldelisle · 2024-06-29T15:15:34Z

I implemented new linting for the .dockstore.yml. I generate a warning if the workflow has no name or no authors as a consequence the test CmdWorkflowLintTestCase is not passing anymore while it is running with --skip best_practices. Edit: ~~Should I integrate these warnings into the best practices?~~ (I cannot because best practices are for workflow files not .dockstore.yml currently).

lldelisle · 2024-06-29T15:17:31Z

Also I am wondering how many tests I should generate following these changes?

bernt-matthias

Cool. I think adding a test for the --iwc flag would be good.

planemo/commands/cmd_workflow_lint.py

planemo/workflow_lint.py

…hias-bernt

…_practices

lldelisle · 2024-07-01T13:54:19Z

Hi,
Here is a summary of this PR:
I have defined:

2 new linters for directories:
- required_files: check that there is a CHANGELOG.md, a README.md, a .dockstore.yml
- changelog: check that there is a version defined in the CHANGELOG.md
1 new linter for .dockstore.yml
- dockstore_best_practices which requires tests and authors and name
1 new linter for workflow
- release that checks that the release matches what is in the CHANGELOG.md
  All these new linters are enabled when the argument --iwc is set.
  I have improved the linter dockstore with common errors we identified in iwc.

Importantly, the check for a testParameterFiles in .dockstore has been moved to dockstore_best_practices so in a regular run will not raise a warning anymore.

mvdbeek · 2024-07-01T14:35:40Z

You can run black to fix the linting errors

mvdbeek · 2024-07-01T15:44:21Z

And isort to fix the importing ... in fact we should add the same make format that we have for galaxy

lldelisle · 2024-07-01T15:47:52Z

@mvdbeek can you help me with the last test. How to assert that the command raise an InputError? and that the exception message contains a given string? Should I test on result.error rather than result.output?

mvdbeek · 2024-07-01T15:55:32Z

I'm seeing:

"Unknown linter type(s) [''] in list of linters to be skipped. Known linters ['CitationsMissing', 'CitationsNoText', 'CitationsFound', 'CitationsNoValid', 'CommandMissing', 'CommandEmpty', 'CommandTODO', 'CommandInterpreterDeprecated', 'CommandInfo', 'CWLValid', 'CWLInValid', 'CWLVersionMissing', 'CWLVersionUnknown', 'CWLVersionGood', 'CWLDockerMissing', 'CWLDockerGood', 'CWLDescriptionMissing', 'CWLHelpTODO', 'ToolVersionMissing', 'ToolVersionPEP404', 'ToolVersionWhitespace', 'ToolVersionValid', 'ToolNameMissing', 'ToolNameWhitespace', 'ToolNameValid', 'ToolIDMissing', 'ToolIDWhitespace', 'ToolIDValid', 'ToolProfileInvalid', 'ToolProfileLegacy', 'ToolProfileValid', 'RequirementNameMissing', 'RequirementVersionMissing', 'RequirementVersionWhitespace', 'ResourceRequirementExpression', 'HelpMissing', 'HelpEmpty', 'HelpPresent', 'HelpTODO', 'HelpInvalidRST', 'HelpValidRST', 'InputsNum', 'InputsMissing', 'InputsMissingDataSource', 'InputsDatasourceTags', 'InputsName', 'InputsNameRedundantArgument', 'InputsNameEmpty', 'InputsNameValid', 'InputsNameDuplicate', 'InputsNameDuplicateOutput', 'InputsTypeChildCombination', 'InputsDataFormat', 'InputsDataOptionsMultiple', 'InputsDataOptionsA...lidatorDatasetMetadataEqualValueOrJson', 'ValidatorMetadataCheckSkip', 'ValidatorTableName', 'ValidatorMetadataName', 'ConditionalParamTypeBool', 'ConditionalParamType', 'ConditionalParamIncompatibleAttributes', 'ConditionalWhenMissing', 'ConditionalOptionMissing', 'ConditionalOptionMissingBoolean', 'OutputsMissing', 'OutputsOutput', 'OutputsNameInvalidCheetah', 'OutputsNameDuplicated', 'OutputsLabelDuplicatedFilter', 'OutputsLabelDuplicatedNoFilter', 'OutputsCollectionType', 'OutputsNumber', 'OutputsFormatInput', 'OutputsFormat', 'OutputsFormatSourceIncomp', 'StdIOAbsenceLegacy', 'StdIOAbsence', 'StdIORegex', 'TestsMissing', 'TestsMissingDatasource', 'TestsAssertsMultiple', 'TestsAssertsHasNQuant', 'TestsAssertsHasSizeQuant', 'TestsAssertsHasSizeOrValueQuant', 'TestsExpectNumOutputs', 'TestsParamInInputs', 'TestsOutputName', 'TestsOutputDefined', 'TestsOutputCorresponding', 'TestsOutputCollectionCorresponding', 'TestsOutputCompareAttrib', 'TestsOutputCheckDiscovered', 'TestsOutputCollectionCheckDiscovered', 'TestsOutputCollectionCheckDiscoveredNested', 'TestsOutputFailing', 'TestsExpectNumOutputsFailing', 'TestsHasExpectations', 'TestsNoValid', 'TestsValid', 'XMLOrder', 'XSD']\n"

I would think that probably means we're not correctly handing a default argument in the skip linters case ?

lldelisle · 2024-07-01T15:59:39Z

Yes I wanted to tell @bernt-matthias about this. This is a message I get each time I run planemo workflow_lint but surprisingly it still gives a exit code of 0:

(planemo) (base) ldelisle@HP-EliteBook-840-G8-Notebook-PC:~/Documents/mygit/planemo$ planemo workflow_lint tests/data/wf_repos/basic_format2_dockstore --skip best_practices
Unknown linter type(s) ['best_practices'] in list of linters to be skipped. Known linters []
.. CHECK: Tests appear structurally correct for tests/data/wf_repos/basic_format2_dockstore/basic_format2.gxwf.yml
.. CHECK: All tool ids appear to be valid.
(planemo) (base) ldelisle@HP-EliteBook-840-G8-Notebook-PC:~/Documents/mygit/planemo$ echo $?
0

lldelisle · 2024-07-01T16:01:13Z

And even without skip, the message is very confusing for users:

(planemo) (base) ldelisle@HP-EliteBook-840-G8-Notebook-PC:~/Documents/mygit/planemo$ planemo workflow_lint tests/data/wf_repos/basic_wf_iwc_invalid_version/
Unknown linter type(s) [''] in list of linters to be skipped. Known linters []
.. CHECK: Tests appear structurally correct for tests/data/wf_repos/basic_wf_iwc_invalid_version/Super-simple-workflow.ga
.. CHECK: All tool ids appear to be valid.
(planemo) (base) ldelisle@HP-EliteBook-840-G8-Notebook-PC:~/Documents/mygit/planemo$ echo $?
0

But I don't know if this should be solved in this PR or in another PR.

mvdbeek · 2024-07-01T16:01:21Z

That's a different from the one in the test though.

diff --git a/planemo/lint.py b/planemo/lint.py
index 5b85022de4..c5964428aa 100644
--- a/planemo/lint.py
+++ b/planemo/lint.py
@@ -23,7 +23,7 @@ def build_lint_args(ctx, **kwds):
         skip = ctx.global_config.get("lint_skip", "")
         if isinstance(skip, list):
             skip = ",".join(skip)
-    skip_types = [s.strip() for s in skip.split(",")]
+    skip_types = [s.strip() for s in skip.split(",") if s.strip()]

     for skip_file in kwds.get("skip_file", []):
         with open(skip_file) as f:

fixes the odd test output.

lldelisle · 2024-07-01T16:01:42Z

Because this would require a big rearrangement I think and it is more difficult to review.

lldelisle · 2024-07-01T16:02:41Z

fixes the odd test output.

I can integrate this.

planemo/workflow_lint.py

bernt-matthias · 2024-07-01T17:05:35Z

I'm seeing:

"Unknown linter type(s) [''] in list of linters to be skipped. Known linters ['CitationsMissing', 'CitationsNoText', 'CitationsFound', 'CitationsNoValid', 'CommandMissing', 'CommandEmpty', 'CommandTODO', 'CommandInterpreterDeprecated', 'CommandInfo', 'CWLValid', 'CWLInValid', 'CWLVersionMissing', 'CWLVersionUnknown', 'CWLVersionGood', 'CWLDockerMissing', 'CWLDockerGood', 'CWLDescriptionMissing', 'CWLHelpTODO', 'ToolVersionMissing', 'ToolVersionPEP404', 'ToolVersionWhitespace', 'ToolVersionValid', 'ToolNameMissing', 'ToolNameWhitespace', 'ToolNameValid', 'ToolIDMissing', 'ToolIDWhitespace', 'ToolIDValid', 'ToolProfileInvalid', 'ToolProfileLegacy', 'ToolProfileValid', 'RequirementNameMissing', 'RequirementVersionMissing', 'RequirementVersionWhitespace', 'ResourceRequirementExpression', 'HelpMissing', 'HelpEmpty', 'HelpPresent', 'HelpTODO', 'HelpInvalidRST', 'HelpValidRST', 'InputsNum', 'InputsMissing', 'InputsMissingDataSource', 'InputsDatasourceTags', 'InputsName', 'InputsNameRedundantArgument', 'InputsNameEmpty', 'InputsNameValid', 'InputsNameDuplicate', 'InputsNameDuplicateOutput', 'InputsTypeChildCombination', 'InputsDataFormat', 'InputsDataOptionsMultiple', 'InputsDataOptionsA...lidatorDatasetMetadataEqualValueOrJson', 'ValidatorMetadataCheckSkip', 'ValidatorTableName', 'ValidatorMetadataName', 'ConditionalParamTypeBool', 'ConditionalParamType', 'ConditionalParamIncompatibleAttributes', 'ConditionalWhenMissing', 'ConditionalOptionMissing', 'ConditionalOptionMissingBoolean', 'OutputsMissing', 'OutputsOutput', 'OutputsNameInvalidCheetah', 'OutputsNameDuplicated', 'OutputsLabelDuplicatedFilter', 'OutputsLabelDuplicatedNoFilter', 'OutputsCollectionType', 'OutputsNumber', 'OutputsFormatInput', 'OutputsFormat', 'OutputsFormatSourceIncomp', 'StdIOAbsenceLegacy', 'StdIOAbsence', 'StdIORegex', 'TestsMissing', 'TestsMissingDatasource', 'TestsAssertsMultiple', 'TestsAssertsHasNQuant', 'TestsAssertsHasSizeQuant', 'TestsAssertsHasSizeOrValueQuant', 'TestsExpectNumOutputs', 'TestsParamInInputs', 'TestsOutputName', 'TestsOutputDefined', 'TestsOutputCorresponding', 'TestsOutputCollectionCorresponding', 'TestsOutputCompareAttrib', 'TestsOutputCheckDiscovered', 'TestsOutputCollectionCheckDiscovered', 'TestsOutputCollectionCheckDiscoveredNested', 'TestsOutputFailing', 'TestsExpectNumOutputsFailing', 'TestsHasExpectations', 'TestsNoValid', 'TestsValid', 'XMLOrder', 'XSD']\n"

I would think that probably means we're not correctly handing a default argument in the skip linters case ?

Fixed in #1453

bernt-matthias · 2024-07-01T17:10:14Z

This: Known linters [] is a problem... I will submit a fix in a separate PR.

bernt-matthias · 2024-07-01T17:14:54Z

#1465

lldelisle · 2024-07-02T06:44:55Z

The only test failing seems unrelated to this PR.

bernt-matthias · 2024-07-02T06:48:53Z

planemo/workflow_lint.py

@@ -59,6 +60,12 @@ class WorkflowLintContext(LintContext):
    training_topic = None


+def build_wf_lint_args(ctx, **kwds):
+    lint_args = build_lint_args(ctx, **kwds)
+    lint_args["iwc_grade"] = str(kwds.get("iwc", False))


Suggested change

lint_args["iwc_grade"] = str(kwds.get("iwc", False))

lint_args["iwc_grade"] = kwds.get("iwc", False)

We should just leave this a boolean.

If I keep it as boolean then I get a type violation as lint_args is supposed to be a dict of str -> str

See

planemo/planemo/workflow_lint.py

Line 125 in b9e62ef

ctx: "PlanemoCliContext", paths: Iterable[str], lint_args: Dict[str, Union[str, List[str]]]

Or I change it to : lint_args: Dict[str, Union[str, List[str], bool]] ?

Dict[str, Any] is probably enough, I'll push a commit to your branch.

Thanks, if not I can commit it.

planemo/workflow_lint.py

Co-authored-by: M Bernt <m.bernt@ufz.de>

mvdbeek

This is excellent, thanks everyone!

lldelisle added 2 commits June 29, 2024 12:29

increase worflow linting

64c684c

linting

af6758e

lldelisle added 6 commits June 29, 2024 14:38

restore build_lint_args

1d61ca1

fix cases with no authors in dockstore.yml

dd9b1bb

fix DOCKSTORE_REGISTRY_CONF_VERSION

f7401d8

change name from required to recommanded

d4ae50f

linting

619cae9

fix forgotten consequence of name from required to recommanded

eb86e3b

bernt-matthias reviewed Jul 1, 2024

View reviewed changes

planemo/commands/cmd_workflow_lint.py Outdated Show resolved Hide resolved

planemo/workflow_lint.py Outdated Show resolved Hide resolved

planemo/workflow_lint.py Outdated Show resolved Hide resolved

lldelisle added 9 commits July 1, 2024 10:26

put iwc_grade in lint_args as suggested by @bernt-matthias

5516443

remove release/version from WorkflowLintContext as suggested by @matt…

8639d1d

…hias-bernt

fix type of lint_args

5c19e56

remove iwc_grade from WorkflowLintContext and add lint_dockstore_best…

302c504

…_practices

new build_wf_lint_args to match what is happening with tools

1310eb7

fix error on CHANGELOG get version

906c5ca

add tests

39480a8

restore comma

7bb3a50

test --iwc on file

db8f24c

remove path from asserts output

22112e8

run black

147296a

run isort

f5af20f

fix strange output when no --skip

6e8f0ce

mvdbeek reviewed Jul 1, 2024

View reviewed changes

planemo/workflow_lint.py Show resolved Hide resolved

fix final test

f748c11

lldelisle added 3 commits July 1, 2024 22:50

change strategy for iwc on files

0d8dbd6

Merge remote-tracking branch 'upstream/master' into lint_workflow

f88024c

add comma

64f9076

lldelisle changed the title ~~[WIP] increase worflow linting~~ Increase worflow linting Jul 2, 2024

bernt-matthias reviewed Jul 2, 2024

View reviewed changes

nsoranzo and others added 4 commits July 2, 2024 12:15

Add type annotation

bafecff

Apply suggestions from @bernt-matthias

b1eedf4

Co-authored-by: M Bernt <m.bernt@ufz.de>

fix lint_args typing to different places

ecdcf0f

remove unused Union

2f7c139

mvdbeek approved these changes Jul 4, 2024

View reviewed changes

mvdbeek merged commit f47965e into galaxyproject:master Jul 4, 2024
13 of 14 checks passed

mvdbeek mentioned this pull request Sep 24, 2024

Check lint skip types only for tool linting #1465

Closed

lldelisle deleted the lint_workflow branch September 24, 2024 07:18

lldelisle mentioned this pull request Sep 27, 2024

Improvement of CI galaxyproject/iwc#297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase worflow linting #1463

Increase worflow linting #1463

lldelisle commented Jun 29, 2024

lldelisle commented Jun 29, 2024

lldelisle commented Jun 29, 2024 •

edited

Loading

lldelisle commented Jun 29, 2024

bernt-matthias left a comment

lldelisle commented Jul 1, 2024

mvdbeek commented Jul 1, 2024 •

edited

Loading

mvdbeek commented Jul 1, 2024

lldelisle commented Jul 1, 2024

mvdbeek commented Jul 1, 2024

lldelisle commented Jul 1, 2024

lldelisle commented Jul 1, 2024

mvdbeek commented Jul 1, 2024 •

edited

Loading

lldelisle commented Jul 1, 2024

lldelisle commented Jul 1, 2024

bernt-matthias commented Jul 1, 2024

bernt-matthias commented Jul 1, 2024

bernt-matthias commented Jul 1, 2024

lldelisle commented Jul 2, 2024

bernt-matthias Jul 2, 2024

lldelisle Jul 2, 2024

lldelisle Jul 2, 2024

lldelisle Jul 2, 2024

nsoranzo Jul 2, 2024

lldelisle Jul 2, 2024

mvdbeek left a comment

	lint_args["iwc_grade"] = str(kwds.get("iwc", False))
	lint_args["iwc_grade"] = kwds.get("iwc", False)

Increase worflow linting #1463

Increase worflow linting #1463

Conversation

lldelisle commented Jun 29, 2024

lldelisle commented Jun 29, 2024

lldelisle commented Jun 29, 2024 • edited Loading

lldelisle commented Jun 29, 2024

bernt-matthias left a comment

Choose a reason for hiding this comment

lldelisle commented Jul 1, 2024

mvdbeek commented Jul 1, 2024 • edited Loading

mvdbeek commented Jul 1, 2024

lldelisle commented Jul 1, 2024

mvdbeek commented Jul 1, 2024

lldelisle commented Jul 1, 2024

lldelisle commented Jul 1, 2024

mvdbeek commented Jul 1, 2024 • edited Loading

lldelisle commented Jul 1, 2024

lldelisle commented Jul 1, 2024

bernt-matthias commented Jul 1, 2024

bernt-matthias commented Jul 1, 2024

bernt-matthias commented Jul 1, 2024

lldelisle commented Jul 2, 2024

bernt-matthias Jul 2, 2024

Choose a reason for hiding this comment

lldelisle Jul 2, 2024

Choose a reason for hiding this comment

lldelisle Jul 2, 2024

Choose a reason for hiding this comment

lldelisle Jul 2, 2024

Choose a reason for hiding this comment

nsoranzo Jul 2, 2024

Choose a reason for hiding this comment

lldelisle Jul 2, 2024

Choose a reason for hiding this comment

mvdbeek left a comment

Choose a reason for hiding this comment

lldelisle commented Jun 29, 2024 •

edited

Loading

mvdbeek commented Jul 1, 2024 •

edited

Loading

mvdbeek commented Jul 1, 2024 •

edited

Loading