Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SW-164] Add ruff and black formatting to spot_wrapper #84

Merged
merged 8 commits into from
Jan 31, 2024

Conversation

khughes-bdai
Copy link
Collaborator

@khughes-bdai khughes-bdai commented Jan 26, 2024

Change Overview

This PR enforces ruff and black linting for all files in spot_wrapper. Pre-commit hooks have been added, and all existing code that did not meet these style requirements has been modified to do so. The configuration files added were taken from the spot_ros2 repository.

Testing Done

I tested a few general Spot examples (move the arm, walk forward) that rely on spot_wrapper and the functionality was unchanged.

Copy link
Collaborator Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

Copy link
Collaborator Author

khughes-bdai commented Jan 26, 2024

TODO this is still failing black tests on the runner for wrapper_helpers.py

@heuristicus
Copy link
Collaborator

Is there a need to use a line length of 120 characters rather than the default of 88? Details at https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#line-length.

Copy link
Collaborator Author

Is there a need to use a line length of 120 characters rather than the default of 88? Details at https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#line-length.

The 120 character line length is also used in spot_ros2 as well as some of our internal repositories, which was where I got that number from. If it's updated here, would we also want to update those other repos?

@bhung-bdai
Copy link
Collaborator

@khughes-bdai internally we follow googles style guide mostly. I think we should go with the default/previously enforced limit in the public facing spot_wrapper. Was there a specific standard previously @heuristicus ?

@heuristicus
Copy link
Collaborator

heuristicus commented Jan 29, 2024

I thought it might be related to internal BDAI policy, and I have no objection if you want to use 120 length to conform to that. I was previously using black on this repository and on spot_ros with default settings. The code itself didn't follow any style guide, although function comments do use the google style.

@bhung-bdai
Copy link
Collaborator

@heuristicus Introducing these precommits will implicitly enforce the Google style. If you are ok with that, I will ask @khughes-bdai to add a link to the style guide on the README

@heuristicus
Copy link
Collaborator

Sure, sounds good.

Copy link
Collaborator

@bhung-bdai bhung-bdai left a comment

Choose a reason for hiding this comment

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

Can you add a link to the Google style guide and a quick blurb about it being the default style?

pyproject.toml Outdated Show resolved Hide resolved
@khughes-bdai khughes-bdai merged commit 6b68c50 into main Jan 31, 2024
3 checks passed
@khughes-bdai khughes-bdai deleted the khughes/ruff_and_black branch January 31, 2024 20:36
@amessing-bdai amessing-bdai mentioned this pull request Feb 2, 2024
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.

4 participants