[22.05] Fix assertion linting to not fail on byte suffixes #15873
Conversation
n, min, max, ... accept bytes (eg 500k) i.e. the type annotation and thereby linting was wrong I suggest to remove it, since its tested in the xsd linter anyway (otherwise we would check for str)
the functions always get a str (from the xml attributes)
| min: Optional[int] = None, | ||
| max: Optional[int] = None, | ||
| negate: bool = False, | ||
| n: Optional[str] = None, |
There was a problem hiding this comment.
Only when coming from the XML right? YAML tools, workflow tests, etc... will be properly typed I imagine. I would use Union types to capture this if it is important to lose the typing for XML.
There was a problem hiding this comment.
Yeah - this feels like you're throwing out a lot of good stuff to support a kind of edge case. Can you just update the size parameter to have a union type and make sure the annotation check is skipping complex types?
There was a problem hiding this comment.
Have added bool/int to the parameters in addition to str.
I'm not sure about the code that I removed from the linting code. Since this is only for xml I'm unsure if its really needed. Feels a bit redundant with the xsd linter. Also for xml we will always have str .. so I don't see this failing in any case.
What do you think?
There was a problem hiding this comment.
Any thoughts on this @jmchilton ? Test failures seem unrelated.
bernt-matthias
force-pushed
the
topic/fix-assertion-bytes
branch
from
a8cf66a
to
fd7963b
Compare
|
This PR was merged without a "kind/" label, please correct. |
mvdbeek
changed the title
fails for instance here: galaxyproject/tools-iuc#5223
The linter checked the type annotations of the assertion function. Since the type annotation was wrong (int does not allow for bytes .. and the functions get str from the xml attribues anyway) linting fails. I would suggest to remove the test .. also because its still covered by the xsd linter.
I could move the 2nd commit to a separate PR.
How to test the changes?
(Select all options that apply)
License