Fix space in graph line string simpler approach #2644
Conversation
|
Out of curiousity, what is Codacy using for its review? pycodestyle doesn't show the issue it mentions for me. Should I just make the test method that it shows up an |
|
If you expand the issue identified in Codacy, it explains more about it, and right at the bottom it says PyLint is the culprit. Yes, looks like you can just make it a class method. |
|
Travis CI is fine for the tests in this case. |
|
My initial feeling - without a close look yet - is I prefer this simpler PR on grounds of clarity and maintainability. |
49ab749 to
7854c5c
Compare
|
I agree. The other one was not maintainable. But, it was very interesting for me to make. I definitely learnt a bit about regex. I have updated with |
7854c5c to
eff6b3c
Compare
|
I'm confused about the pycodestyle failure in the travis ci process. From what I can see looking at the suggested approaches for indentation, it should work? When I run pycodestyle on my local it doesn't warn about anything either. |
|
(I don't understand the Travis CI failure either - |
|
All looking good again. |
lib/cylc/graph_parser.py
Outdated
| # Apparently this is the fastest way to strip all whitespace!: | ||
| line = "".join(line.split()) | ||
| if not line: | ||
| modifiedLine = self.__class__.REC_COMMENT.sub('', line) |
There was a problem hiding this comment.
Please use modified_line.
https://www.python.org/dev/peps/pep-0008/#function-and-variable-names
| @@ -270,6 +290,18 @@ def parse_graph(self, graph_string): | |||
| # If debugging, print the final result here: | |||
| # self.print_triggers() | |||
|
|
|||
| @classmethod | |||
| def _report_invalid_lines(cls, lines): | |||
There was a problem hiding this comment.
Please add doc string.
There was a problem hiding this comment.
I thought I had sent this question already, but apparently I didn't.
Is there a preferred pydoc style? reST? Google? Numpy? I think I've seen Google a few times, but this file I don't think matches any of the standard ones?
There was a problem hiding this comment.
We follow PEP8 for style. The docstring section https://www.python.org/dev/peps/pep-0008/#documentation-strings links out to PEP257. However, we're not very consistent on exact docstring format (blanks lines before and after final triple-quotes for example) - if you write one that looks more or less like others in graph_parser.py that'll be fine.
There was a problem hiding this comment.
Thanks. I've put in something.
lib/cylc/graph_parser.py
Outdated
| (?!\s*[\-+]\s*[0-9]) # allow spaces before/after -+ if numbers follow | ||
| \s+ # do not allow 'task<space>task' | ||
| {task_suf} | ||
| '''.format(task=TaskID.NAME_RE, task_suf=TaskID.NAME_SUFFIX_RE), re.X) |
There was a problem hiding this comment.
Not sure whether it is worth using .format here, which may complicate things.
eff6b3c to
eaf63f4
Compare
|
Note we might want to go with #2628 instead of this, for #2646 (comment) |
Currently, the graph parsing will not raise an error when there are spaces between task names, as spaces get stripped out `foo => bar baz` becomes `foo => barbaz`. It can be captured if that task does not exist, but if there was a task with that name, it could lead to some odd results. This change takes a simple approach to stop the space separated task validation problems, and also to prevent bad spaces in parameters and around triggers. It will not capture all potential validation problems, some of which are covered by later validation checks. It was not possible to just do task names and to ignore paramters and triggers, as they generally follow the same format.
eaf63f4 to
912074c
Compare
This fixes #2534. It is a much simpler and less strict approach than was used in #2628 (which can be closed in favour of this one if preferred). The simplest approach of looking for
TaskID.NAME_REdoes not work because you can have hyphens with numbers after them quite reasonably, at least in a parameter. I couldn't think of a clean way to only have this checkout outside of parameter expansion, or to ignore triggers likesucceed-all, so it hits them all. Thus, why I had to put in extra regex to ignore-[0-9]with spaces around the minus sign, those are obviously fine to have happen.I will note though, that because of the approach described above, if something like
task -23ortask + 23is done, it won't catch it, and it will just pass. But, given a task name must start with a\w, it is probably not too bad, and less of a problem than currently exists. I don't have this failure mode covered in the tests.I'll also admit here, I don't actually have myself setup to run the full test battery, so I've only run these unit tests and will be relying on Travis to point out any problems.