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

[ENH] add a .git-blame-ignore-revs to ignore formatting PR from git blame #3653

Open
1 of 2 tasks
Remi-Gau opened this issue Apr 4, 2023 · 11 comments · Fixed by #3663
Open
1 of 2 tasks

[ENH] add a .git-blame-ignore-revs to ignore formatting PR from git blame #3653

Remi-Gau opened this issue Apr 4, 2023 · 11 comments · Fixed by #3663
Assignees
Labels
Effort: low The issue is likely to require a small amount of work (less than a few hours). Enhancement for feature requests Impact: low Solving this issue will have a small impact on the project. Maintenance This issue is related to maintenance work.

Comments

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Apr 4, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Describe your proposed enhancement in detail.

Came across while browsing the black documentation:
https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html#avoiding-ruining-git-blame

One problem with all the reformating PRs is that they make reduce the usefulness (or at least add more friction) to the use of git blame to trace change was introduced when: black may end up touching many if not most lines in the code base and git blame may invariably point to one of those reformatting PRs when we use it, even though it very unlikely that a behavioral change came from them.

Using a .git-blame-ignore-revs could allow to "skip" to PR during a git blame call.

  • ignore PRs for f strings ?

Benefits to the change

Ideally this should facilitate the tracking down of changes in the code base by ignoring changes that pure reformatting and do not entail functional change.

@Remi-Gau Remi-Gau added the Enhancement for feature requests label Apr 4, 2023
@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Apr 4, 2023

Let me know what you think.

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Apr 4, 2023

Also it seems to also work when using git blame via the github interface: https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/

@Remi-Gau Remi-Gau changed the title [ENH] add a .git-blame-ignore-revs to ignore formatting PR from git blame [ENH] add a .git-blame-ignore-revs to ignore formatting PR from git blame Apr 4, 2023
@ymzayek
Copy link
Member

ymzayek commented Apr 5, 2023

I already feel that it's a bit slower to track down changes so I would be for this, but I haven't used it before. What would be the downside if any?

@NicolasGensollen
Copy link
Member

I never used it but I'm +1 on this.
The risk and cost seem very small and I can clearly see the benefits of having a meaningful blame where @Remi-Gau isn't responsible for all the bugs of Nilearn 😉.

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Apr 5, 2023

I can clearly see the benefits of having a meaningful blame where @Remi-Gau isn't responsible for all the bugs of Nilearn

Rémi then proceeds to blame-ignore ALL the PRs he ever authored... 😜

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Apr 18, 2023

See this comment: #3701 (comment)

Also keep this issue opened until most of the reformatting is not over

@Remi-Gau
Copy link
Collaborator Author

Related to #3701 (comment)

 git blame --ignore-revs-file .git-blame-ignore-revs nilearn/image/resampling.py 

copy paste of a comment from #3755

Still shows content from 3651 event though it is supposed to be ignored:

# run isort and flake8 on the whole nilearn code base in CI (#3651)
44933309ecd09add795ce1c3ea5edaab4294da87

More annoyingly it is also still the case for:

 git blame --ignore-rev 44933309  nilearn/image/resampling.py

@Remi-Gau Remi-Gau added Maintenance This issue is related to maintenance work. Impact: low Solving this issue will have a small impact on the project. Effort: low The issue is likely to require a small amount of work (less than a few hours). labels Jun 28, 2023
@ghisvail
Copy link

We use one of these in Clinica.

@ymzayek
Copy link
Member

ymzayek commented Dec 12, 2023

To add:

#4147 25c456f34d45409f07dcca560e45f892a05dde4b
#4146 724cd51672417fd7db1e11c0f755178130df9feb
#4145 e083dba365ebf6a60bc9cd2e6b979030c769e4bf
#4144 7d4d7f3a343c68ed44e0d356b65ba78af4444f3a

@Remi-Gau
Copy link
Collaborator Author

Thanks.
Though I am still at a loss to actually make this git blame ignore work as I think it should.

@ymzayek
Copy link
Member

ymzayek commented Dec 12, 2023

I'm also unsure of how to make it work :/ If I get a chance I'll look into it some more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort: low The issue is likely to require a small amount of work (less than a few hours). Enhancement for feature requests Impact: low Solving this issue will have a small impact on the project. Maintenance This issue is related to maintenance work.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants