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

Update pre-commit #2094

Merged
merged 6 commits into from
Apr 13, 2023
Merged

Update pre-commit #2094

merged 6 commits into from
Apr 13, 2023

Conversation

Shobuj-Paul
Copy link
Contributor

@Shobuj-Paul Shobuj-Paul commented Apr 8, 2023

Description

Updated pre-commit hooks and ran them in the repo.
Fixes #2093

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials/documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link

codecov bot commented Apr 8, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.17 🎉

Comparison is base (884813d) 50.67% compared to head (20ac55d) 50.83%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2094      +/-   ##
==========================================
+ Coverage   50.67%   50.83%   +0.17%     
==========================================
  Files         391      391              
  Lines       32167    32167              
==========================================
+ Hits        16298    16350      +52     
+ Misses      15869    15817      -52     
Impacted Files Coverage Δ
...de/moveit_setup_controllers/controllers_widget.hpp 0.00% <ø> (ø)
...oveit_setup_controllers/src/controllers_widget.cpp 0.00% <ø> (ø)

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ibrahiminfinite
Copy link
Contributor

If you change "Fixes issue" to just "Fixes" it will automatically link the issue as a target for closing.
As you can see it has not currently been linked.

@Shobuj-Paul
Copy link
Contributor Author

Thanks, I have made the change.

@Shobuj-Paul
Copy link
Contributor Author

Tried updating clang-format-14 to clang-format-15, but there is some major conflict which I could not understand so reverted the commits

@Shobuj-Paul
Copy link
Contributor Author

Shobuj-Paul commented Apr 10, 2023

Can someone please help me with this? Updating clang-format-15 causes issues with rolling-ci+ikfast workflow. Logs show that it is failing to clone. How are the two things related?
Or am I missing something else entirely?
@JafarAbdi

@JafarAbdi
Copy link
Member

@Shobuj-Paul Please revert the change to the clang-format version, we use a system installed one and changing it might cause issues with other people

@Shobuj-Paul
Copy link
Contributor Author

@JafarAbdi
Even after rolling it back to the previous commit, the same test fails. I am not sure why it worked before but is not working now.

@vatanaksoytezer
Copy link
Contributor

@Shobuj-Paul I think ikfast failure is not related to your changes:

Error downloading object: benchmarking/databases/panda_kitchen_test_db.sqlite (b6b06bb): Smudge error: Error downloading benchmarking/databases/panda_kitchen_test_db.sqlite (b6b06bbbbe0eb6fe0dacc6d44386051b4d48e884b77a256697912ecdd09f528c): batch response: This repository is over its data quota. Account responsible for LFS bandwidth should purchase more data packs to restore access.

@Shobuj-Paul
Copy link
Contributor Author

@Shobuj-Paul I think ikfast failure is not related to your changes:

Error downloading object: benchmarking/databases/panda_kitchen_test_db.sqlite (b6b06bb): Smudge error: Error downloading benchmarking/databases/panda_kitchen_test_db.sqlite (b6b06bbbbe0eb6fe0dacc6d44386051b4d48e884b77a256697912ecdd09f528c): batch response: This repository is over its data quota. Account responsible for LFS bandwidth should purchase more data packs to restore access.

I thought as much, thanks for confirming.

Copy link
Member

@JafarAbdi JafarAbdi 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 follow-ups! just one issue and I'll merge

The changes to the .eps files will break them, right? I think we should ignore them in the spell-checker hook

@Shobuj-Paul
Copy link
Contributor Author

Thanks for the follow-ups! just one issue and I'll merge

The changes to the .eps files will break them, right? I think we should ignore them in the spell-checker hook

My bad, I didn't think even the .eps files will be edited. I have excluded all .eps files in the pre-commit configuration and reverted the changes from before.

Copy link
Member

@JafarAbdi JafarAbdi left a comment

Choose a reason for hiding this comment

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

Thanks!

@JafarAbdi JafarAbdi merged commit 3e847e1 into moveit:main Apr 13, 2023
@Shobuj-Paul Shobuj-Paul deleted the update-pre-commit branch April 13, 2023 15:02
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.

Updating the pre-commit hooks in configuration files
4 participants