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

Configure and apply pre-commit #280

Merged
merged 4 commits into from
Apr 5, 2022
Merged

Conversation

jcarpent
Copy link
Contributor

@jcarpent jcarpent commented Apr 2, 2022

Related to #279.

@jcarpent jcarpent force-pushed the devel branch 2 times, most recently from 350c399 to 5f1486c Compare April 2, 2022 15:29
@nim65s
Copy link
Contributor

nim65s commented Apr 2, 2022

While we're at it, I think we should also apply black to reformat python code, by adding this section to .pre-commit-config.yaml:

-   repo: https://github.com/psf/black
    rev: 22.3.0
    hooks:
    -   id: black

@jcarpent jcarpent force-pushed the devel branch 3 times, most recently from 9005aa5 to f904404 Compare April 2, 2022 22:29
@jmirabel
Copy link
Contributor

jmirabel commented Apr 4, 2022

I would like that people looking at git blame understand that they are looking at a code reformatting commit and not an actual change. To this end, I propose to

  • to reword commit pre-commit: init all to something more explicit. Maybe Reformat code to Google style
  • modify the author of the message to (Reformat) Justin Carpentier. I am asking because depending on how you configure git blame, you don't have the commit message right away.

@nim65s
Copy link
Contributor

nim65s commented Apr 4, 2022

We can also exclude these reformating commits from git blame, with --ignore-rev / --ignore-revs-file. A .git-blame-ignore-revs file, which can be versioned in the repo, seems to a common convention: https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame / https://akrabat.com/ignoring-revisions-with-git-blame/

@jmirabel
Copy link
Contributor

jmirabel commented Apr 4, 2022

We can also exclude these reformating commits from git blame, with --ignore-rev / --ignore-revs-file. A .git-blame-ignore-revs file, which can be versioned in the repo, seems to a common convention: https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame / https://akrabat.com/ignoring-revisions-with-git-blame/

Didn't know about this. Indeed, that makes code reformatting a lot more attractive !

@nim65s
Copy link
Contributor

nim65s commented Apr 4, 2022

Then I suggest we merge this, and release. Should I handle that ?

@jmirabel
Copy link
Contributor

jmirabel commented Apr 4, 2022

Don't you want to add the git blame file first ?
Then, yes, it's good for me.

@nim65s
Copy link
Contributor

nim65s commented Apr 4, 2022

sure

To use it, either:
- git blame --ignore-revs-file .git-blame-ignore-revs
- git config blame.ignoreRevsFile .git-blame-ignore-revs
@nim65s
Copy link
Contributor

nim65s commented Apr 4, 2022

PS: Github support for this feature is still in beta: community/community#5033

But it looks like this is alrealy working:
https://github.com/jcarpent/hpp-fcl/blame/devel/include/hpp/fcl/collision.h :
blame

@wxmerkt
Copy link
Contributor

wxmerkt commented Apr 4, 2022

Should we expand the README on how to use pre-commit and to manually set the git-blame setting (per commit message it needs to be set manually on every developer machine)?

[add git blame ignore file](https://github.com/humanoid-path-planner/hpp-fcl/pull/280/commits/5fea4840a47ec1b895757f6ecc5bdc691ae1535a)
[5fea484](https://github.com/humanoid-path-planner/hpp-fcl/pull/280/commits/5fea4840a47ec1b895757f6ecc5bdc691ae1535a)

To use it, either:
- git blame --ignore-revs-file .git-blame-ignore-revs
- git config blame.ignoreRevsFile .git-blame-ignore-revs

@nim65s
Copy link
Contributor

nim65s commented Apr 4, 2022

I'm not sure how useful it would be to add more docs about those standard tools here.

They are becoming pretty common, and for anybody who is not aware of those, github UI will automatically provide the right blame, and pre-commit.ci will automatically add commits to re-format if needed.

But I won't oppose if someone provide that doc :)

@jcarpent
Copy link
Contributor Author

jcarpent commented Apr 4, 2022

I can handle the release soon also.

@jcarpent jcarpent merged commit 6b8a549 into coal-library:devel Apr 5, 2022
@jcarpent
Copy link
Contributor Author

jcarpent commented Apr 5, 2022

Before doing the new release, I will fix this issue #267.

@jcarpent
Copy link
Contributor Author

jcarpent commented Apr 5, 2022

The new release version will thus be 2.0.0

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