Skip to content

Conversation

@akx
Copy link
Contributor

@akx akx commented Apr 4, 2024

This makes GitHub (and other tools that support this file, or any Git client with git config blame.ignoreRevsFile .git-blame-ignore-revs set) ignore the large reformatting/automatic commits, to make blame inspection easier.

@akx
Copy link
Contributor Author

akx commented May 27, 2024

@Pierre-Sassoulas Added the file to MANIFEST.in too to make the check Tox step pass.

@akx akx requested a review from Pierre-Sassoulas May 27, 2024 16:51
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

76fb2a6 could be removed from the git blame but not ff3fdd8 which introduce some non formatting changes (remove six, maybe others) and some unsafe fixes from ruff. I'm pretty sure the migration from os.path to pathlib is not automated either (I initially thought it was because of the MR ff3fdd8 was introduced in, until I actually tried and the deception was strong).

@akx akx force-pushed the git-blame-ignore-revs branch from 50d5cee to aff4437 Compare August 26, 2024 09:35
@akx akx requested a review from Pierre-Sassoulas August 26, 2024 09:35
@webknjaz
Copy link
Member

I'd argue it's not just a formatting change but a functional one. F-strings are more performant so it's more of an enhancement rather than formatting. Usually only patches that are solely formatting are ignored. Bits that are useful for git paleontology aren't supposed to be hidden...

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I'm not sure what's the point of .git-blame-ignore-revs in Manifest.in : after packaging git is not involved anymore, right ? So if f-strings removed from git blame are not consensual, there would be nothing left of this MR if we also remove that 😅

@akx
Copy link
Contributor Author

akx commented Aug 26, 2024

I'm not sure what's the point of .git-blame-ignore-revs in Manifest.in

Without it there, check-manifest would fail.

@Pierre-Sassoulas
Copy link
Member

What is check-manifest and why does it fail ? manifest.in is linked to python packaging and does not involve git, right ?

@ionelmc
Copy link
Member

ionelmc commented Aug 26, 2024

check-manifest is used to make sure the sdist contains all the source files and nothing is missing. Maybe this new file needs to be an exclude in there.

But alas, I am not sure who is this file for. I will not use it - I prefer to see everything in the gitblame, and jumping back a commit is pretty easy enough (it's just one button in github anyway).
image

@webknjaz
Copy link
Member

webknjaz commented Oct 9, 2024

But alas, I am not sure who is this file for. I will not use it - I prefer to see everything in the gitblame, and jumping back a commit is pretty easy enough (it's just one button in github anyway). image

@ionelmc this file is picked up by GitHub automatically, and it hides the listed commits from blame, unlike local Git invocations where that needs to be configured.

@webknjaz
Copy link
Member

webknjaz commented Oct 9, 2024

check-manifest is used to make sure the sdist contains all the source files and nothing is missing. Maybe this new file needs to be an exclude in there.

Have you considered using setuptools-scm which auto-includes everything Git-tracked into sdists?

@webknjaz
Copy link
Member

@ionelmc would you be open to integrating setuptools-scm?

@ionelmc
Copy link
Member

ionelmc commented Mar 31, 2025

@webknjaz I am not opposed in principle. I use it on projects with C extensions where it's useful to have uniquely versioned pre-release distributions. But on this project I don't see the value to change an already working packaging setup. As a matter of fact I only see myself responding to bug reports caused by such a change.

Anyway, regarding this ignore-revs change - I don't want it. I simply don't trust even myself to not introduce behavior changes in formatting commits.

@ionelmc ionelmc closed this Mar 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants