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

Separate the installation of pre-commit to -f|--format case #28

Merged
merged 3 commits into from
Mar 2, 2023
Merged

Separate the installation of pre-commit to -f|--format case #28

merged 3 commits into from
Mar 2, 2023

Conversation

AndrejOrsula
Copy link
Contributor

Description

This PR separates the installation of pre-commit from the main installation via orbit.sh -i|--install into the already existing -f|--format case.

This means that end-users are no longer required to install pre-commit when using the included orbit.sh -i|--install option, which should simplify the Dockerfile layers (#17).

With this PR, pre-commit is automatically installed with orbit.sh -f|--format option if its command is not detected on the system. Furthermore, this PR makes sure that pre-commit run is always executed in the correct directory regardless of ${PWD} when calling orbit.sh -f|--format . Contributors are already informed to run pre-commit via the PR checklist found in the template (as seen below).

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have run the pre-commit checks with pre-commit run --all-files (see here instructions to set it up)
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file

Signed-off-by: Andrej Orsula <orsula.andrej@gmail.com>
Signed-off-by: Andrej Orsula <orsula.andrej@gmail.com>
orbit.sh Show resolved Hide resolved
@Mayankm96
Copy link
Contributor

Signed-off-by: Andrej Orsula <orsula.andrej@gmail.com>
@AndrejOrsula
Copy link
Contributor Author

We should probably make doc changes at the following places to only use the bash utilities instead of pre-commit. What do you think?

I added instructions to use ./orbit.sh --format instead of pre-commit run --all-files in those three files. I don't know if there's a better way of formatting it (I think that ./orbit.sh --format is more informative than ./orbit.sh -f).
In one of the files, I also removed a link to the installation instructions of pre-commit as it is done automatically via ./orbit.sh -f|--format now.

Please, review this change (ceaf2a0).

@Mayankm96
Copy link
Contributor

Yeah, I agree. The flag --format is more explicit for users.

I slowly want to make the dependency on being on top of the repo to run orbit.sh a bit obsolete. The solution to add this script to the ~/.bashrc is alright but I think it doesn't scale well when one has multiple clones of the repo.

Anyway, that's for another discussion. I'll merge and close this one for now. Thanks a lot!

@Mayankm96 Mayankm96 merged commit 9eb7810 into isaac-sim:main Mar 2, 2023
@AndrejOrsula AndrejOrsula mentioned this pull request Mar 2, 2023
5 tasks
Batou1406 added a commit to iit-DLSLab/dls_orbit that referenced this pull request Jul 8, 2024
Merge 'sampler' branch into main
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.

2 participants