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

Code formatting in CI #59

Merged
merged 12 commits into from
Jan 12, 2023
Merged

Conversation

ibrahiminfinite
Copy link
Collaborator

@ibrahiminfinite ibrahiminfinite commented Jan 11, 2023

Will close #55

@ibrahiminfinite ibrahiminfinite marked this pull request as draft January 11, 2023 08:10
@sea-bass
Copy link
Owner

sea-bass commented Jan 11, 2023

Thanks for kicking this off!

This might end up being a bigger PR than expected, since adding a GitHub action to run Black means all the code will have to be auto-formatted :)

@ibrahiminfinite
Copy link
Collaborator Author

The Github action seems to be working for black.
It is currently set to fail the workflow if files need to be edited.

@ibrahiminfinite
Copy link
Collaborator Author

@sea-bass
The diff output for Black looks good to me (https://github.com/sea-bass/pyrobosim/actions/runs/3898671363/jobs/6657630984)
If this looks okay to you , we can proceed to enable the auto-formatting.

Copy link
Owner

@sea-bass sea-bass 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 getting this started!

I meant to do this and have a lot of particular preferences (sorry), so here you go :)

.pre-commit-config.yaml Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.github/workflows/black.yml Outdated Show resolved Hide resolved
.github/workflows/black.yml Outdated Show resolved Hide resolved
@ibrahiminfinite
Copy link
Collaborator Author

Looks like it is working directly with pre-commit now.
The diffs are also looking good.

@sea-bass
Copy link
Owner

Looks like it is working directly with pre-commit now. The diffs are also looking good.

Yes they are, this is so cool to see!

I'll leave it up to you if you want to actually do all the formatting changes; else, we can merge this as-is and I can update with the suggestions in a follow-on PR.

@ibrahiminfinite
Copy link
Collaborator Author

Do you mean the formatting changes suggested by Black ?

Copy link
Owner

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Small stuff.

Again, I'm happy to approve it after this if you don't want to actually commit the formatting suggestions as part of this PR, but up to you.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.github/workflows/format.yml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
@ibrahiminfinite
Copy link
Collaborator Author

Small stuff.

Again, I'm happy to approve it after this if you don't want to actually commit the formatting suggestion as part of this PR, but up to you.

No problem. will add the suggested changes by Black to this PR itself.

@sea-bass
Copy link
Owner

No problem. will add the suggested changes by Black to this PR itself.

Maybe you already know this, but just in case!

If you install pre-commit (https://pre-commit.com/) and then run this from the repo root,

pre-commit run -a

it will do all the changes automatically.

@ibrahiminfinite
Copy link
Collaborator Author

ibrahiminfinite commented Jan 12, 2023

The Format workflow is failing since this branch was made before the world modeling tests were merged and so does not include the 'test_world.py' which is in main.
Might have to fix formatting for the test_world.py and world.py in another PR.
Or is it possible to fix it here ?

@sea-bass
Copy link
Owner

sea-bass commented Jan 12, 2023

You can merge my repo's main branch into yours. Let me do that right now!

Also, I guess pre-commit doesn't automatically add shebangs to shell scripts. Added those too.

@sea-bass sea-bass marked this pull request as ready for review January 12, 2023 04:25
@ibrahiminfinite
Copy link
Collaborator Author

It passed !
That is cool

@sea-bass
Copy link
Owner

If the test_pddlstream_manip unit tests fail (they have been recently), it's something hopefully fixed by #57, so we can merge this anyway.

Thank you so much for doing this!

@sea-bass sea-bass merged commit 8f40083 into sea-bass:main Jan 12, 2023
@ibrahiminfinite ibrahiminfinite deleted the code_formatter branch January 13, 2023 05:42
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.

Add code formatting checks to CI
2 participants