Skip to content

Conversation

christophebedard
Copy link
Member

No description provided.

Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
@christophebedard christophebedard requested a review from tfoote as a code owner May 13, 2024 20:31
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me with green CI.

Out of curiosity, how did you notice these? And why didn't they get picked up by our linters in here?

@christophebedard
Copy link
Member Author

christophebedard commented May 13, 2024

Out of curiosity, how did you notice these?

I've been syncing our internal code to newer versions of packages over the past few months, so I've been looking closely at a ton of packages and contributing PRs for minor stuff like this. In this case, a linter that we usually only use for internal code was unintentionally run on this package, so I noticed these parts.

And why didn't they get picked up by our linters in here?

That's a good question 😆 flake8 and pep257 do check those files:

PASSED
test/test_flake8.py::test_flake8 
8 files checked
No problems found

Checked files:

* ./setup.py
* ./test/test_flake8.py
* ./test/test_pep257.py
* ./test/test_copyright.py
* ./test/test_point_cloud2.py
* ./sensor_msgs_py/numpy_compat.py
* ./sensor_msgs_py/point_cloud2.py
* ./sensor_msgs_py/__init__.py
PASSED
test/test_pep257.py::test_pep257 checking ./sensor_msgs_py/__init__.py
checking ./sensor_msgs_py/numpy_compat.py
checking ./sensor_msgs_py/point_cloud2.py
checking ./setup.py
checking ./test/test_copyright.py
checking ./test/test_flake8.py
checking ./test/test_pep257.py
checking ./test/test_point_cloud2.py
checking test/test_copyright.py
checking test/test_flake8.py
checking test/test_pep257.py
checking test/test_point_cloud2.py
No problems found
PASSED

If I make more abhorrent style changes, the tests fail as expected, so I guess they're really OK with how it was before even though most of us wouldn't really format code like that.

I was also a bit irked by the double indentation of function arguments in point_cloud2.py:

cloud: PointCloud2,
field_names: Optional[List[str]] = None,
skip_nans: bool = False,
uvs: Optional[Iterable] = None,
reshape_organized_cloud: bool = False) -> np.ndarray:
. However, again, the tests pass, and it's probably just a matter of style.

@christophebedard
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup. It's odd to have snuck through for this long.

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ahcorde ahcorde merged commit 0e0450b into ros2:rolling May 14, 2024
@christophebedard christophebedard deleted the christophebedard/fix-sensor-msgs-py-fmt branch May 14, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants