Comments more comments! #18
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
luca-della-vedova
requested changes
Jul 1, 2024
There was a problem hiding this comment.
I don't quite have the rights to approve the run of the workflow but I tested locally and this makes the CI linter fail:
$ cat build/colcon-ros-cargo/pytest.xml
<?xml version="1.0" encoding="utf-8"?><testsuites><testsuite name="colcon-ros-cargo" errors="0" failures="1" skipped="0" tests="1" time="0.575" timestamp="2024-07-01T09:53:04.257939" hostname="ros.lucadv1"><testcase classname="colcon-ros-cargo.test.test_flake8" name="test_flake8" time="0.468"><failure message="AssertionError: flake8 reported 56 errors assert not 56">test/test_flake8.py:53: in test_flake8
assert not total_errors, f'flake8 reported {total_errors} errors'
E AssertionError: flake8 reported 56 errors
E assert not 56</failure></testcase></testsuite></testsuites>
I suspect this is due to the line splits being reverted for long lines, the quote style going from single to double.
Can you make sure your PR doesn't make colcon test fail for this package?
|
if your linter can't tell you exactly where errors occur, it's difficult to do anything about it. I can't just change things at the drop of a hat. root@steamdeck:/ros_debug_ws/src/colcon-ros-cargo# colcon test
Starting >>> colcon-ros-cargo
[0.897s] ERROR:colcon.colcon_core.task.python.test:Failed to find the following files:
- /ros_debug_ws/src/colcon-ros-cargo/install/colcon-ros-cargo/share/colcon-ros-cargo/package.sh
Check that the following packages have been built:
- colcon-ros-cargo
Failed <<< colcon-ros-cargo [0.01s, exited with code 1]
Summary: 0 packages finished [0.71s]
1 package failed: colcon-ros-cargoTo be honest, this output leaves me rather perplexed. The said |
|
Hem OK, that came up a bit rude but I'm sure that wasn't the intention. The comments are great but your PR does a lot of other changes that are unrelated, make the diff explode and are actually the main reason the code style test is failing. Before trying to run Anyway the test itself is a flake8 check with a few suppressed warnings, you can also run it as a CLI utility, this is in your branch: By contrast this is what it currently outputs on main: Note that the |
|
I also wanted to modernise the code. Then, when I get round to it, I'll just delete all the commits that change the code and push it again. Basically, the python interpreter doesn't care whether you make single or double quotes, for example. For Python, there is only one |
esteve
requested changes
Aug 29, 2024
| @@ -34,10 +34,25 @@ class AmentCargoBuildTask(CargoBuildTask): | |||
| """ | |||
|
|
|||
| def __init__(self): # noqa: D107 | |||
| """ | |||
There was a problem hiding this comment.
This comment is just a line-by-line read of the code, doesn't add anything
Watch your tone, @luca-della-vedova gave you constructive feedback. The linter we use is the most common one (flake8), familiarize yourself with the Python tooling and conventions (pep8), and next time try to be more polite. Thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
added more pydoc comments