Provide even more context to the lint context #13186

bernt-matthias · 2022-01-18T19:34:27Z

Follow up on #12978

main improvement is the addition of the source file (the tool file or a macro file). Note that this is the full path (so in a planemo run this will be /tmp/...)
Also added a mechanism to extent lint messages.
In order to make this usable from planemo or language server I added get_lint_context_for_tool_source and changed lint_tool_source_with and lint_xml_with to return the lint context.

Also added a first test for linting yaml and cwl tools (and fixed minor bugs)

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.

davelopez · 2022-01-19T11:51:14Z

There is one thing to do: In order to make this usable from planemo or language server I would like to change at least one of the functions in lib/galaxy/tool_util/lint.py

I was wondering if we can further refactor it to separate the linting from the printing in LintContext. I haven't looked at the code in detail, but maybe calling LintContext.lint should just populate the state of the LintContext and add the proper LintMessages, etc, and then, calling LintContext.print will print out the messages. That way we can add a new function get_lint_context (or something similar) that will return the fully populated context (without printing anything) and that can be used in the existing lint_* functions without having to return the context.

From the point of view of the language server or other services that want information on linting but do their own printing, just calling get_lint_context from the tool XML will return the fully populated LintContext and they can work with that.

What do you think?

bernt-matthias · 2022-01-19T12:17:04Z

What do you think?

Sounds like a good idea.

lib/galaxy/tool_util/parser/yaml.py

bernt-matthias · 2022-01-25T12:54:01Z

@mvdbeek if this does not make it to 22.01 (which would be a pitty) I think that #12978 should be reverted and the commits added here.

mvdbeek · 2022-01-25T12:55:49Z

Let's get it into 22.01 then.

mvdbeek · 2022-01-25T12:57:18Z

lib/galaxy/tool_util/linters/stdio.py

-        tool_line = 0
-        tool_path = None
+    # determine node to report for general problems with stdio
+    tool_node = tool_xml.find("./tool")


We're allowing tool_xml to be None here, so I guess you want to guard this with a if tool_xml is not None: ?

I would suggest to move the linter tests for yaml and cwl to a separate PR.

The problem here is that the main lint function should distinguish which linters to call based on the tool language and not the tool type. Thus, all the xml linters are called for the yaml tools.

Also with the suggested change the tool_type is still None (because the yaml file does not include it).

in preparation to get linter tests running for such tools cwl: - remove a print from SchemaLoader yaml: - define YamlToolSource.parse_tool_type otherwise yaml tools are parsed like xml tools - parse_version: return a string, otherwise 1.0 is treated as float which makes the linter (and probably more) fail

while parsing macros

- add fname to LintMessage - LintContext make lint messages persistent over multiple linter calls before the messages were reset before the call of the next linter which is "impractical" if we want to use the lintmessages eg in planemo - add test for linting a tool + (internal and external) macros to ensure that the fname and line number is correct - add a simple linter test for yaml and json tools

davelopez

could you have a look if this is useful

Yeah! this is what I had in mind, I can't wait to try and integrate it in GLS :)

I just added some comments mostly about typing, but they are just minor improvements.

lib/galaxy/tool_util/lint.py

davelopez · 2022-01-27T09:40:26Z

lib/galaxy/tool_util/lint.py

@@ -89,59 +124,67 @@ def __init__(self, level, skip_types=None, object_name=None):
        self.skip_types = skip_types or []
        self.level = level
        self.object_name = object_name
-        self.found_errors = False
-        self.found_warns = False
+        self.message_list = []


Suggested change

self.message_list = []

self.message_list: List[LintMessage] = []

For this, we will need to import from typing import List but again it is nicer to be explicit about the type here

I added type annotations at most places. Maybe you can recheck?

Thank you! That's a lot of type hints 😆
Check mypy errors, usually, they give a nice hint to fix them.
I would use just None instead of NoReturn for methods.
As per the documentation:

NoReturn is a special type to annotate functions that never return normally. For example, a function that unconditionally raises an exception.

lib/galaxy/tool_util/lint.py

subclassed XMLLintMessage

bernt-matthias · 2022-01-27T15:55:15Z

I think it's better to pass the parameter to the error call, and have the lint_ctx accept a function that gets you the lines.
That way you can adapt this to any tool language by providing a different function, and you don't need to call node_props everywhere

@mvdbeek Guess I have found a solution .. subclassing: LintMessage

Special LintMessage for XML: - one adding line and file name and - one adding XPath added error and report level class also added doc string

lib/galaxy/tool_util/lint.py

Co-authored-by: Marius van den Beek <m.vandenbeek@gmail.com>

github-actions · 2022-01-28T18:32:30Z

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

bernt-matthias · 2022-01-28T19:06:49Z

Thanks @mvdbeek

mvdbeek · 2022-01-28T19:15:45Z

Thanks for all your work on the linting and parameters in general, this is really cool!

error introduced here galaxyproject#13186 found this while working on galaxyproject/planemo#1264

github-actions bot added area/testing area/tool-framework area/util labels Jan 18, 2022

github-actions bot added this to the 22.01 milestone Jan 18, 2022

bernt-matthias added status/WIP and removed area/testing area/tool-framework area/util labels Jan 18, 2022

github-actions bot added area/testing area/tool-framework area/util labels Jan 18, 2022

bernt-matthias mentioned this pull request Jan 18, 2022

Provide more context for lint context #12978

Merged

11 tasks

jmchilton approved these changes Jan 19, 2022

View reviewed changes

bernt-matthias marked this pull request as draft January 19, 2022 14:10

mvdbeek reviewed Jan 25, 2022

View reviewed changes

lib/galaxy/tool_util/parser/yaml.py Outdated Show resolved Hide resolved

mvdbeek modified the milestones: 22.01, 22.05 Jan 25, 2022

mvdbeek reviewed Jan 25, 2022

View reviewed changes

mvdbeek modified the milestones: 22.05, 22.01 Jan 25, 2022

bernt-matthias added 6 commits January 26, 2022 09:50

small restructuring of linter unit tests

7fcc772

lint for invalid regex in stdio regex

4e237d8

fix node.base to macro file

b7ef626

while parsing macros

xsd: make name attribute required

977f0fa

davelopez reviewed Jan 27, 2022

View reviewed changes

add type annotations

33807be

bernt-matthias force-pushed the topic/lint-context2 branch 4 times, most recently from 3373a22 to f3ba64e Compare January 27, 2022 14:34

refactoring of LintMessage

f3ba64e

subclassed XMLLintMessage

bernt-matthias force-pushed the topic/lint-context2 branch 2 times, most recently from 5c75e70 to 665d83e Compare January 27, 2022 16:13

eliminate node_props

665d83e

bernt-matthias force-pushed the topic/lint-context2 branch 4 times, most recently from 212d77c to 8f94fbe Compare January 28, 2022 16:14

more linting refactoring

8f94fbe

Special LintMessage for XML: - one adding line and file name and - one adding XPath added error and report level class also added doc string

mvdbeek approved these changes Jan 28, 2022

View reviewed changes

lib/galaxy/tool_util/lint.py Outdated Show resolved Hide resolved

Fix typo

1bc2b4f

Co-authored-by: Marius van den Beek <m.vandenbeek@gmail.com>

mvdbeek merged commit 4fe4737 into galaxyproject:dev Jan 28, 2022

mvdbeek modified the milestones: 22.05, 22.01 Jan 28, 2022

mvdbeek added the kind/enhancement label Jan 28, 2022

bernt-matthias deleted the topic/lint-context2 branch January 28, 2022 19:06

bernt-matthias mentioned this pull request Jan 31, 2022

Planemo lint fails in 22.01 for imported macros #13277

Closed

bernt-matthias mentioned this pull request Feb 14, 2022

[22.01] Fix macro parsing #13373

Merged

5 tasks

bernt-matthias added a commit to bernt-matthias/galaxy that referenced this pull request Aug 23, 2022

fix lint context fail function

0b3f8af

error introduced here galaxyproject#13186 found this while working on galaxyproject/planemo#1264

bernt-matthias mentioned this pull request Aug 23, 2022

[22.01] fix lint context fail function #14496

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide even more context to the lint context #13186

Provide even more context to the lint context #13186

bernt-matthias commented Jan 18, 2022 •

edited

Loading

davelopez commented Jan 19, 2022

bernt-matthias commented Jan 19, 2022

bernt-matthias commented Jan 25, 2022

mvdbeek commented Jan 25, 2022

mvdbeek Jan 25, 2022

bernt-matthias Jan 26, 2022 •

edited

Loading

davelopez left a comment

davelopez Jan 27, 2022

bernt-matthias Jan 27, 2022

davelopez Jan 27, 2022

bernt-matthias commented Jan 27, 2022

github-actions bot commented Jan 28, 2022

bernt-matthias commented Jan 28, 2022

mvdbeek commented Jan 28, 2022

	self.message_list = []
	self.message_list: List[LintMessage] = []

Provide even more context to the lint context #13186

Provide even more context to the lint context #13186

Conversation

bernt-matthias commented Jan 18, 2022 • edited Loading

How to test the changes?

License

davelopez commented Jan 19, 2022

bernt-matthias commented Jan 19, 2022

bernt-matthias commented Jan 25, 2022

mvdbeek commented Jan 25, 2022

mvdbeek Jan 25, 2022

Choose a reason for hiding this comment

bernt-matthias Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

davelopez left a comment

Choose a reason for hiding this comment

davelopez Jan 27, 2022

Choose a reason for hiding this comment

bernt-matthias Jan 27, 2022

Choose a reason for hiding this comment

davelopez Jan 27, 2022

Choose a reason for hiding this comment

bernt-matthias commented Jan 27, 2022

github-actions bot commented Jan 28, 2022

bernt-matthias commented Jan 28, 2022

mvdbeek commented Jan 28, 2022

bernt-matthias commented Jan 18, 2022 •

edited

Loading

bernt-matthias Jan 26, 2022 •

edited

Loading