Conversation
clalancette
left a comment
clalancette
left a comment
There was a problem hiding this comment.
This looks structurally reasonable. I've left some things to consider fixing in the Python code, but it is really nothing major.
| help='The files or directories to check. For directories files ending ' | ||
| 'in %s will be considered.' % |
There was a problem hiding this comment.
| help='The files or directories to check. For directories files ending ' | |
| 'in %s will be considered.' % | |
| help='The files or directories to check. For each directory only files ending ' | |
| 'in %s will be considered.' % |
(or something like that; having directories files looks weird)
Also, if you list a directory is it searched recursively?
| # not using a file handle directly | ||
| # in order to prevent leaving an empty file when something fails early |
There was a problem hiding this comment.
It's not clear to me what this comment refers to.
| for path in paths: | ||
| if os.path.isdir(path): | ||
| for dirpath, dirnames, filenames in os.walk(path): | ||
| if 'AMENT_IGNORE' in dirnames + filenames: |
There was a problem hiding this comment.
This is a question, not a statement: should we extend this to honor COLCON_IGNORE as well?
There was a problem hiding this comment.
I stumbled across this and my two cents:
This should absolutely honor COLCON_IGNORE as well.
| def invoke_yamllint(files, yamllint_bin=None, config_file=None): | ||
| if yamllint_bin is None: | ||
| yamllint_bin = find_executable('yamllint') |
There was a problem hiding this comment.
It seems unnecessary to have yamllint_bin be an argument here; there are no situations where we use it. I guess I would just remove it and just unconditionally do yamllint_bin = find_executable('yamllint')
| temp_config_fd, temp_config_file = tempfile.mkstemp(suffix='.yaml', | ||
| prefix='yamllint_') | ||
| with os.fdopen(temp_config_fd, 'w') as f: | ||
| yaml.dump(yamllint_config, f) | ||
|
|
||
| try: | ||
| report = invoke_yamllint(files, config_file=temp_config_file) | ||
| except: # noqa: E722 | ||
| raise | ||
| finally: | ||
| os.remove(temp_config_file) |
There was a problem hiding this comment.
I think this whole thing might be better done with a NamedTemporaryFile in a context. That way we are always sure it is removed, even if yaml.dump, for instance, throws an exception.
| except subprocess.CalledProcessError as e: | ||
| if e.stderr: | ||
| print(e.stderr.decode()) | ||
| stdout = e.stdout.decode() | ||
| print(stdout, end='') | ||
| if not stdout or e.returncode not in (1, 2): | ||
| raise | ||
| parser = re.compile(r'^(.+):(\d+):(\d+): \[(.+)\] (.*) \((.+)\)$') | ||
| for line in stdout.splitlines(): | ||
| m = parser.match(line) | ||
| if not m: | ||
| raise ValueError( | ||
| 'Failed to parse yamllint output: %s' % (line,)) | ||
| try: | ||
| report[m.group(1)].append({ | ||
| 'line': int(m.group(2)), | ||
| 'col': int(m.group(3)), | ||
| 'severity': m.group(4), | ||
| 'msg': m.group(5), | ||
| 'id': m.group(6), | ||
| }) | ||
| except NameError: | ||
| raise ValueError( | ||
| 'Got failures for unknown file: %s' % (m.group(1),)) |
There was a problem hiding this comment.
I'm not a huge fan of doing tons of work inside of an exception handler. While Python 3 is far better at dealing with this than Python 2 was, it can still be confusing to read the backtrace where multiple exceptions happened inside of one another. My preferred solution here is to just check what we need to if we intend to re-raise, then save off what we need in the exception handler and continue on. All other work can be done in the "main" context by checking whether any data was saved from the exception handler.
That said, I don't feel that strongly about it, so if you don't think this is worthwhile, I won't block on it.
The yamllint tool is widely used to validate YAML style. This change adds ament_lint support for that tool. At present, the ament_lint_auto glue for ament_cmake_yamllint is omitted. We'll need to clean up all of the violations across the codebase before we can enable it automatically. For now, having the tool available may help us prevent further regressions. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
force-pushed
the
cottsay/yamllint
branch
from
9dd5a9a to
98d49c4
Compare
|
@cottsay Are you still working on this? |
|
Is my understanding correct that |
This introduces ament_yamllint and ament_cmake_yamllint packages to support YAML linting in ROS 2. This work revives and finalizes the original proposal in PR ament#351. Co-authored-by: Scott K Logan <logans@cottsay.net>
This introduces ament_yamllint and ament_cmake_yamllint packages to support YAML linting in ROS 2. This work revives and finalizes the original proposal in PR ament#351. Co-authored-by: Scott K Logan <logans@cottsay.net> Signed-off-by: wj <frozenreboot@gmail.com>
This introduces ament_yamllint and ament_cmake_yamllint packages to support YAML linting in ROS 2. This work revives and finalizes the original proposal in PR ament#351. Co-authored-by: Scott K Logan <logans@cottsay.net> Signed-off-by: wj <frozenreboot@gmail.com>
4 participants
The yamllint tool is widely used to validate YAML style. This change adds ament_lint support for that tool.
At present, the ament_lint_auto glue for ament_cmake_yamllint is omitted. We'll need to clean up all of the violations across the
codebase before we can enable it automatically.
For now, having the tool available may help us prevent further regressions.
Requires: ros2/ci#613
CI: