Split linters in separate classes

[bernt-matthias]

So far mainly a call for discussion. Would be nice to have this discussed first before I continue with the other linters
(discuss we could transform only stdio, a few more, or all linters within this PR).

Currently, a linter is a function prefixed by lint_ (most of them are contained in tool_util.linters which are used by default). These linters are at the moment quite monolithic, e.g. there is a single linter for the input tag and we have
only a limited number of states (check, info, warn, error).

This led to a lot of problems and frustration (see) since

• It has never been defined what qualifies as an error/warning and contributors had their own different definitions.
• It has not been possible to disable single checks, but only complete linters (which is not useful due to their monolithic nature)

So far this change does the following

• Create a new Linter class which makes it easier to decide what a linter is in the loop over the modules.
• The main method of this class is the classmethod lint
• We could have a fix method that may be used to fix some problems (discuss: maybe not in this PR?).
• when I started I added a code property (inspired by pep8/flake8 codes) which we could add to the LintMessages that are generated by the lint methods. But then I realised that skipping of linters simply works by the linter name (i.e. class name) .. maybe this is sufficient (discuss: advantage of codes is that we definitely have short codes for the planemo CLI, alphanumeric codes might become longer but this is closer to what we have at the moment). Alternatively we could just call the classes with codes, e.g. S001.

The following already works: planemo lint tools/filters/cutWrapper.xml --skip StdIOAbsence

• removes the attribute check for regex and exit_code tags and subtag check for stdio since those are covered already by the xsd linter.

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[mvdbeek]

Don't we need to have a code per message, unless one linter only emits one possible message ?

[bernt-matthias]

We can try to do this. With the current state I would need to split StdIOAbsence only, or? I hoped to come away with something like at most one message per linter (which would be fine for profile versions as in StdIOAbsence.

[bernt-matthias]

Also, it's important to note that we currently filter for linter function/class names (here) and not linter message codes. I would say that do not need the codes. But we can either name the filter classes using a code.

[mvdbeek]

One message per linter sounds fine, but also like a lot of work.

[bernt-matthias]

One message per linter sounds fine, but also like a lot of work.

The good thing is that we have good unit test coverage - which now pays off.

[mvdbeek]

If you augment the lint_ctx.function to instead pass codes we can use those codes to determine what is classified as an error / what can be ignored. This seems like less work (we'd have to run all relatively coarse linters, but I think that's ok ?)

[bernt-matthias]

Excellent idea, but I think we are fixed to the signature of the linter function, i.e 1st parameter ToolSource and 2nd parameter LintContext. The reason is that there are some linters that live in the planemo sources. One option would be to move them here (not sure if this would add extra requirements) - which might be good anyway.

But there might be another way, due to this hack we can filter the messages before adding them to the final message list (and also if without this hack we could filter the list of messages after each linter application).

I'm a bit worried how we can ensure that our codes are not duplicated.

One message per linter sounds fine, but also like a lot of work.

Main question for me is what is the "best" solution ..

we'd have to run all relatively coarse linters, but I think that's ok ?

I also think this would be OK. Runtime should not be a concern.

[jmchilton]

The reason is that there are some linters that live in the planemo sources

I think when I first implemented the XSD validation the external dependency on xmllint (https://github.com/galaxyproject/planemo/blob/master/planemo/xml/validation.py#L14C20-L14C27) was the issue. lxml seemed less mature at the time than xmllint. I guess we use lxml a lot more aggressively now and we have been generally pleased with the XSD validation it is doing and we now have a Galaxy dependency on lxml. I think I would be fine dropping the xmllint path through planemo if it would simplify things and let the XSD validation move into Galaxy.

[bernt-matthias]

Thanks for the comment @jmchilton. Will try to move it here, but probably in a separate PR.

I'm still experimenting if separate classes are the way to go. It seems difficult to avoid code duplication, but it will become clearer how we end up in the different linter messages.

[bernt-matthias]

Ping @mvdbeek and @jmchilton I'm quite happy with the current state. I have moved the lxml-based xsd validation from planemo to here. I thought it would be a good idea to run the xsd linter with each of the unit tests for the linters (mainly to remove redundant linters) which uncovered a few minor problems (e.g. in the xsd).

For me, the TODOs would be

• decide if we want to store the linter type with each message and if we want shorter (eg numeric) linter names
  • we could also have something like aliases which would also allow to continue to use current skip lists, e.g. "inputs"
• run the current and previous state of the linters on the IUC repo (maybe also devteam)
• adapt planemo
  • remove the lxml based linter (should we keep the possibility to lint with xmllint?)
  • maybe allow to specify a skip list from file
  • maybe new sub command to list all available linters + doc

Regarding lxml I was also wondering if we should remove https://github.com/galaxyproject/galaxy/blob/e7a168000e627dc8579ad749174bd5eabbe15873/lib/galaxy/util/__init__.py#L68C9-L68C9

[mvdbeek]

I think that's actually a rare case where you can use a Metaclass:

In [1]: linters = {}
   ...:
   ...: class LinterMeta(type):
   ...:     def __new__(cls, clsname, bases, attrs):
   ...:         newclass = super(LinterMeta, cls).__new__(cls, clsname, bases, attrs)
   ...:         linters[clsname] = newclass
   ...:         return newclass
   ...:
   ...:
   ...: class MyLinter(metaclass=LinterMeta):
   ...:     pass
   ...:
   ...: linters
Out[1]: {'MyLinter': __main__.MyLinter}

[bernt-matthias]

Thanks for pointing me to metaclasses. Never took the time to learn about them.

Still I hope that 3a98a3a is sufficient?

[mvdbeek]

Any chance you could resolve the conflicts ?

[mvdbeek]

Thanks a lot @bernt-matthias, this is really important work!

[github-actions]

This PR was merged without a "kind/" label, please correct.