[23.0] Allow duplicate labels in linter if outputs contain filters #15933
Conversation
bernt-matthias
force-pushed
the
topic/fix-output-label-linter
branch
from
a05e10b
to
a81f095
Compare
| node=output | ||
| ) | ||
| else: | ||
| lint_ctx.error(f"Tool output [{name}] uses duplicated label '{label}'", node=output) |
There was a problem hiding this comment.
Both should probably just be warnings ? Functionality is not impacted, which I think is what errors should be about.
| filter_node = output.find(".//filter") | ||
| if filter_node is not None: | ||
| lint_ctx.info( | ||
| f"Tool output [{name}] uses duplicated label '{label}', double check if filters imply disjoint cases", |
There was a problem hiding this comment.
Just had it here: galaxyproject/tools-iuc#5291 .. but found it acceptable in this case to use different labels. Can't remember a really good example.
Point is that we (probably?) need to allow for the possibility to have two outputs that are never outputed together (i.e. they use disjoint filters). Then it might OK to allow for the same label. For instance if from_work_dir needs to be different when a flag is set or not set.
Since it seemed pretty hard (or impossible) to check the filters if they are disjoint I thought that its best to just check if there is a filter.
There was a problem hiding this comment.
Then it might OK to allow for the same label. For instance if
from_work_dirneeds to be different when a flag is set or not set.
I'd move the output to the right place or use the same output name, which is much friendlier for predicting what outputs will be generated. I think a warning is just the right level here.
There was a problem hiding this comment.
If we make this a warning then the outcome (IUC wise) would be the same (galaxyproject/tools-iuc#5260), ie. the PR workflow would fail.
But, given your idea to skip linting for the push to master/main we could just raise the fail level to error for the merge.
Just tell me your preference (warning / info) and I will change it.
There was a problem hiding this comment.
warning is the right level IMO, galaxyproject/tools-iuc#5291 was improved by the new label and I think in many / all instances there are tweaks that would make the tool better as in either more userfriendly or more predictable
There was a problem hiding this comment.
How about this example @mvdbeek: galaxyproteomics/tools-galaxyp#721 ?
There was a problem hiding this comment.
I'm sorry, can you point out what I should look at there ?
There was a problem hiding this comment.
There is an output dataset and an output collection which are mutually exclusive. They have the same name which I find acceptable in this case.
There was a problem hiding this comment.
ok, so you're also saying it should be a warning ?
There was a problem hiding this comment.
Yes, according to my intuition of the meaning of "warning".
|
Ready from my side. Test failures seem unrelated to me. |
I still think warning is the right thing to do. what we do with warnings in the IUC is another question that has more than one answer. |
|
Or let's rephrase it, it sure shouldn't be an error, if you can move the info to a warning we can merge this and refine later if necessary. I'd also target 23.0 at this point. |
bernt-matthias
force-pushed
the
topic/fix-output-label-linter
branch
from
111ba2e
to
c684e0b
Compare
|
OK. Rebased. Guess for IUC we should disable the linter in the PR workflow runs for the merge to master? |
duplicated labels may be OK if there are filters
bernt-matthias
changed the title
mvdbeek
changed the title
duplicated labels may be OK if there are filters
How to test the changes?
(Select all options that apply)
License