Provide more context for lint context #12978

bernt-matthias · 2021-11-24T17:08:06Z

The tool linter currently "only" provides a message. It might be cool if the output would allow more easily to find the problem. Here I try to add the source line / xpath to the linter messages.

This would be particularly helpful if integrated in 3rd party software like the GLS: galaxyproject/galaxy-language-server#180

TODO:

extend the lint_tool_source function to return the lint context(s)
test if macros influence line numbers / xpath
get source file (https://bugs.launchpad.net/lxml/+bug/1952737)
check if #include works
- this is cheetah specific, ie unrelated
Full unit test coverage for linters, i.e., test the output of each linter message
lint_general gets tool_source and determines requirements not via xml foo, so extended output is not yet possible
warning / error specific codes and a possibility to filter for them.
- another time :)

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 contributions under Galaxy's current license.
I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to galaxy-committers@lists.galaxyproject.org.

bernt-matthias · 2021-11-24T20:03:12Z

Hah, this looks fancy:

Linting tool /home/berntm/projects/tools-iuc/tool_collections/bamtools/bamtools_split/bamtools_split.xml
Applying linter tests... FAIL
.. ERROR: Test param output_type not found in the inputs (line 62) [/tool/tests/test[2]/param[3]]
.. CHECK: 2 test(s) found. (line 48) [/tool/tests/test[2]]
Applying linter stdio... CHECK
.. INFO: No stdio definition found, tool indicates error conditions with output written to stderr. (line 0)
Applying linter output... CHECK
.. INFO: 1 outputs found. (line 43)
Applying linter inputs... CHECK
.. INFO: Found 3 input parameters. (line 1)
Applying linter help... CHECK
.. CHECK: Tool contains help section. (line 1) [/tool]
.. CHECK: Help contains valid reStructuredText. (line 71) [/tool/help]
Applying linter general... CHECK
.. CHECK: Tool defines a version [2.4.0]. (line 0)
.. CHECK: Tool defines a name [Split]. (line 0)
.. CHECK: Tool defines an id [bamtools_split]. (line 0)
.. CHECK: Tool targets 16.01 Galaxy profile. (line 0)
Applying linter command... CHECK
.. INFO: Tool contains a command. (line 6) [/tool/command]
Applying linter citations... CHECK
.. CHECK: Found 1 likely valid citations. (line 1) [/tool]
Applying linter tool_xsd... CHECK
.. INFO: File validates against XML schema.
Failed linting

davelopez · 2021-11-25T09:07:43Z

Awesome!
Maybe the extra information output like line number and XPath can be made optional when printing, just in case someone wants the old representation :)

bernt-matthias · 2021-11-26T10:56:53Z

Line and xpath reported seem to be useful out of the box:

line numbers seem to correspond to the actual line numbers in the sources (only its unclear if the tool / the macros file)
xpaths seem to be correct

Here is a test tool + macro and the planemo output:

test.xml

     1	<tool id="test" name="test" version="@TOOL_VERSION@+galaxy@VERSION_SUFFIX@" profile="20.01" license="MIT">
     2	    <description>test</description>
     3	    <macros>
     4	        <token name="@TOOL_VERSION@">0.1.0</token>
     5	        <token name="@VERSION_SUFFIX@">0</token>
     6	        <import>macros.xml</import>
     7	    </macros>
     8	    <xrefs>
     9	        <xref type="bio.tools"></xref>
    10	    </xrefs>
    11	    <requirements>
    12	        <requirement type="package" version="@TOOL_VERSION@">test</requirement>
    13	    </requirements>
    14	    <command detect_errors="exit_code"><![CDATA[
    15	        ## TODO: Fill itestn command using Cheetah templates
    16	        ## Hint: Use [ctrl+alt+c] after defining the inputs/outputs to auto-generate some Cheetah boilerplate code for you.
    17	    ]]></command>
    18	    <inputs>
    19	        <expand macro="matrix_scooler_macro"/>
    20	        <param type="select" name="test"/>
    21	    </inputs>
    22	    <outputs>
    23	    </outputs>
    24	    <tests>
    25	        <!-- Hint: You can use [ctrl+alt+t] after defining the inputs/outputs to auto-scaffold some basic test cases. -->
    26	    </tests>
    27	</tool>

macros.xml

     1	<macros>
     2	    <xml name='matrix_scooler_macro'>
     3	        <param name='1matrix_scooler' type="data" format="txt"
     4	            label="Matrix to compute on"/>
     5	    </xml>
     6	</macros>

planemo lint reports

Linting tool /tmp/test/test.xml
Applying linter tests... WARNING
.. WARNING: No tests found, most tools should define test cases. (line 24) [/tool/tests]
.. WARNING: No valid test(s) found. (line 24) [/tool/tests]
Applying linter output... CHECK
.. INFO: 0 outputs found. (line 22)
Applying linter inputs... FAIL
.. ERROR: Select parameter [test] options have to be defined by either 'option' children elements, a 'options' element or the 'dynamic_options' attribute. (line 20) [/tool/inputs/param[2]]
.. WARNING: Param input [1matrix_scooler] is not a valid Cheetah placeholder. (line 4) [/tool/inputs/param[1]]
.. INFO: Found 2 input parameters. (line 1)
Applying linter help... WARNING
.. WARNING: Help contains TODO text. (line 27) [/tool/help]
.. CHECK: Tool contains help section. (line 27) [/tool/help]
.. CHECK: Help contains valid reStructuredText. (line 27) [/tool/help]
Applying linter general... CHECK
.. CHECK: Tool defines a version [0.1.0+galaxy0]. (line 0)
.. CHECK: Tool defines a name [test]. (line 0)
.. CHECK: Tool defines an id [test]. (line 0)
.. CHECK: Tool specifies profile version [20.01]. (line 0)
Applying linter command... CHECK
.. INFO: Tool contains a command. (line 14) [/tool/command]
Applying linter citations... FAIL
.. ERROR: Empty doi citation. (line 48) [/tool/citations/citation]
Applying linter tool_xsd... CHECK
.. INFO: File validates against XML schema.
Failed linting

davelopez · 2021-11-26T11:24:57Z

Line and xpath reported seem to be useful out of the box

Nice! we can definitely work with that!

Only some things I found confusing:

WARNING: Param input [1matrix_scooler] is not a valid Cheetah placeholder. (line 4) [/tool/inputs/param[1]] This seems to point to the macros.xml file as you said, in this case, the line number seems a bit off, I think it should be 3, but the XPath may be a better hint to place it at line 19 of test.xml

This block seems a bit off too, apparently there is no /tool/help section defined but it is assuming there is one?

Applying linter help... WARNING
.. WARNING: Help contains TODO text. (line 27) [/tool/help]
.. CHECK: Tool contains help section. (line 27) [/tool/help]
.. CHECK: Help contains valid reStructuredText. (line 27) [/tool/help]

Also the same here: ERROR: Empty doi citation. (line 48) [/tool/citations/citation] There is no citations section at all and line 48 doesn't exist 🤔

bernt-matthias · 2021-11-26T12:16:10Z

Nice! we can definitely work with that!

cool

Only some things I found confusing:

WARNING: Param input [1matrix_scooler] is not a valid Cheetah placeholder. (line 4) [/tool/inputs/param[1]] This seems to point to the macros.xml file as you said, in this case, the line number seems a bit off, I think it should be 3, but the XPath may be a better hint to place it at line 19 of test.xml

Maybe lxml takes the line where the tag was closed? One can argue about the xpath (but here I guess lxml already expanded and therefore the expand tag can not be referenced. Would be interesting to see what happens for linter errors in the expand tag .. if this is even possible).

I guess in most cases we have to live with the numbers and paths provided by lxml.

For the others: Seems that I posted outdated output:

Linting tool /tmp/test/test.xml
Applying linter tests... WARNING
.. WARNING: No tests found, most tools should define test cases. (line 24) [/tool/tests]
.. WARNING: No valid test(s) found. (line 24) [/tool/tests]
Applying linter output... CHECK
.. INFO: 0 outputs found. (line 22)
Applying linter inputs... FAIL
.. ERROR: Select parameter [test] options have to be defined by either 'option' children elements, a 'options' element or the 'dynamic_options' attribute. (line 20) [/tool/inputs/param[2]]
.. WARNING: Param input [1matrix_scooler] is not a valid Cheetah placeholder. (line 4) [/tool/inputs/param[1]]
.. INFO: Found 2 input parameters. (line 1)
Applying linter help... WARNING
.. WARNING: No help section found, consider adding a help section to your tool. (line 1) [/tool]
Applying linter general... CHECK
.. CHECK: Tool defines a version [0.1.0+galaxy0]. (line 0)
.. CHECK: Tool defines a name [test]. (line 0)
.. CHECK: Tool defines an id [test]. (line 0)
.. CHECK: Tool specifies profile version [20.01]. (line 0)
Applying linter command... CHECK
.. INFO: Tool contains a command. (line 14) [/tool/command]
Applying linter citations... WARNING
.. WARNING: No citations found, consider adding citations to your tool. (line 1) [/tool]
Applying linter tool_xsd... CHECK
.. INFO: File validates against XML schema.

line number and xpath

ie include only the tested features

- more specific error message for whitespace in pre/suffix in tool version and name - do not pront valid tool id message if whitespaces are found

and several small improvements - lint for empty param name / type - distinct option definition possibilities for selects and selects in conditionals - condtional parameter - check for exactly 1 conditional param (before only 0) - warn if boolean select param for conditional - warn for optional and multiple select

- and add warning if no valid citation is found

- info->warn for deprecated interpreter attribute - add error for empty command text - fix check for TODO in command

- fix warning for invalid cheetah placeholder

bernt-matthias · 2022-01-17T12:49:30Z

Thanks for merging :)

But it was not actually finished yet. I just removed the wip label before the committers meeting in order to get it into the discussion and forgot to readd it.

I will add a follow up PR today or tomorrow.

bernt-matthias · 2022-01-17T21:54:14Z

@davelopez I did some experiments and have two questions:

sourceline refers to the correct line also in external macro files, but it seems impossible to get the name of the file (except for dirty hacks .. ?)
- actually quite nice, I guess that the source line is initialize while parsing and is unaltered when macros are replaced
- may this still be useful for the language server? Otherwise I would remove this .. in particular because I'm unsure if xml from the stdlib supports this at all (or only lxml)
the xpath refers to the element in the xml document after expanding the macros (the macros block is empty after wards)
- still useful?

davelopez · 2022-01-18T10:08:17Z

sourceline refers to the correct line also in external macro files, but it seems impossible to get the name of the file (except for dirty hacks .. ?)

Nice, I think we can work with that. I don't know if it is too common to have multiple macro files but probably there is some guesswork we can do at the language server level. For this, I will have to do some experiments too :)

the xpath refers to the element in the xml document after expanding the macros (the macros block is empty after wards)

Yeah, we can show the expanded version of the document for troubleshooting the linting errors and that will be helpful to point to the correct line in case the sourceline is not so helpful here.

bgruening · 2022-01-18T10:10:08Z

Nice, I think we can work with that. I don't know if it is too common to have multiple macro files but probably there is some guesswork we can do at the language server level. For this, I will have to do some experiments too :)

Not super common, but it exists. @annefou love multiple ones I think :)

annefou · 2022-01-18T10:16:06Z

Yep I do. I am probably not a good example in terms of best practices...

bgruening · 2022-01-18T10:19:10Z

You have complicated tools and it makes sense in those cases I think. Circos is another example: https://github.com/galaxyproject/tools-iuc/tree/master/tools/circos

bernt-matthias · 2022-01-18T12:21:31Z

Good news, seems that node.base contains the source file :)

Just adding this.

bernt-matthias · 2022-01-18T20:46:34Z

Continue here #13186

github-actions bot added the area/tool-framework label Nov 24, 2021

github-actions bot added this to the 22.01 milestone Nov 24, 2021

bernt-matthias mentioned this pull request Nov 24, 2021

Linting support galaxyproject/galaxy-language-server#180

Closed

bernt-matthias force-pushed the topic/lint-context branch from f638f00 to f03b3b2 Compare November 24, 2021 17:15

bernt-matthias marked this pull request as draft November 24, 2021 17:16

bernt-matthias added the status/WIP label Nov 24, 2021

bernt-matthias force-pushed the topic/lint-context branch from 9326829 to e6c5ff6 Compare November 24, 2021 20:02

bernt-matthias force-pushed the topic/lint-context branch from e6c5ff6 to fa55cf8 Compare November 24, 2021 20:36

bernt-matthias force-pushed the topic/lint-context branch 3 times, most recently from 3f8554d to aea2d3e Compare November 30, 2021 10:23

bernt-matthias removed the status/WIP label Jan 14, 2022

bernt-matthias marked this pull request as ready for review January 14, 2022 15:14

bernt-matthias added 12 commits January 17, 2022 11:03

provide more context for lint context

96654f8

line number and xpath

remove bare excepts

a7edfaa

add xpath

1090519

small fix for tests linter

fd3cc41

make linter test xml more minimal

a2ace33

ie include only the tested features

check tool_id only if present

d2903f2

general linter: full test coverage

93b4176

- more specific error message for whitespace in pre/suffix in tool version and name - do not pront valid tool id message if whitespaces are found

full test coverage for help linter

1b5f120

fix rebase error

5b34abf

full test coverage for stdio

9d7f3a1

full test coverage for citations linter

994cc17

- and add warning if no valid citation is found

bernt-matthias added 7 commits January 17, 2022 11:07

full test coverage for command linter

f699676

- info->warn for deprecated interpreter attribute - add error for empty command text - fix check for TODO in command

full test coverage for outputs linter

706048c

- fix warning for invalid cheetah placeholder

full test coverage for xml_order linter

4fe9781

small restructuring of inputs linter test

a73de59

full test coverage for tests linter

16070cc

check number of info and valid messages in all tests

57ab0e6

add TODO for later

c37dcd1

bernt-matthias force-pushed the topic/lint-context branch from efc8d52 to 48abc4a Compare January 17, 2022 10:14

cleanup rebase mistakes

48abc4a

mvdbeek added kind/enhancement kind/feature labels Jan 17, 2022

mvdbeek merged commit be14264 into galaxyproject:dev Jan 17, 2022

bernt-matthias mentioned this pull request Jan 18, 2022

Provide even more context to the lint context #13186

Merged

5 tasks

bernt-matthias deleted the topic/lint-context branch January 18, 2022 20:46

bernt-matthias mentioned this pull request Feb 14, 2022

[22.01] Fix macro parsing #13373

Merged

5 tasks

mvdbeek changed the title ~~provide more context for lint context~~ Provide more context for lint context Feb 15, 2022

davelopez mentioned this pull request Sep 10, 2022

Integrate Galaxy linting galaxyproject/galaxy-language-server#204

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide more context for lint context #12978

Provide more context for lint context #12978

bernt-matthias commented Nov 24, 2021 •

edited

Loading

bernt-matthias commented Nov 24, 2021

davelopez commented Nov 25, 2021

bernt-matthias commented Nov 26, 2021

davelopez commented Nov 26, 2021 •

edited

Loading

bernt-matthias commented Nov 26, 2021

bernt-matthias commented Jan 17, 2022

bernt-matthias commented Jan 17, 2022

davelopez commented Jan 18, 2022

bgruening commented Jan 18, 2022

annefou commented Jan 18, 2022

bgruening commented Jan 18, 2022

bernt-matthias commented Jan 18, 2022

bernt-matthias commented Jan 18, 2022

Provide more context for lint context #12978

Provide more context for lint context #12978

Conversation

bernt-matthias commented Nov 24, 2021 • edited Loading

How to test the changes?

License

bernt-matthias commented Nov 24, 2021

davelopez commented Nov 25, 2021

bernt-matthias commented Nov 26, 2021

davelopez commented Nov 26, 2021 • edited Loading

bernt-matthias commented Nov 26, 2021

bernt-matthias commented Jan 17, 2022

bernt-matthias commented Jan 17, 2022

davelopez commented Jan 18, 2022

bgruening commented Jan 18, 2022

annefou commented Jan 18, 2022

bgruening commented Jan 18, 2022

bernt-matthias commented Jan 18, 2022

bernt-matthias commented Jan 18, 2022

bernt-matthias commented Nov 24, 2021 •

edited

Loading

davelopez commented Nov 26, 2021 •

edited

Loading