Hello 👋,
in#1886 ande7dec7d a proper warning message was introduced for the usage ofDiff.renamed pointing to useDiff.renamed_file.
However, the usage of this property wasn't changed in theiter_change_type (since 3 years at that), so the internals use deprecated code 😞

GitPython/git/diff.py

Lines 328 to 329 in9fbfb71

	elifchange_type=="R"anddiffidx.renamed:
	yielddiffidx

Given that the code wasn't reported yet, perhaps I'm doing something wrong using theiter_change_type and there are better ways 👀 I'm using a custom MkDocs hook to run GitPython and check for renames to automatically create redirects mappings for paths, and another plugin handles the redirect creation for the static pages.
Here is the line which triggered the warning

https://github.com/Gothic-Modding-Community/gmc/blob/b15fc318827d18ae1c2b67a4a73bc3d74c08b0ea/overrides/.hooks/git_redirects.py#L128

And the CI:

https://github.com/Gothic-Modding-Community/gmc/actions/runs/9248064690/job/25437812921#step:8:21

Warning

As this is a one line change, I took the liberty to omit setting up an environment and just used the GitHub GUI to make a small change.
I also didn't investigate deeper to check if theiter_change_type has any tests.

Thanks for your time ✌️

Byron requested changes

May 28, 2024

View reviewed changes

Copy link

Member

Byron left a comment

Choose a reason for hiding this comment

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

Thanks a for investigating this issue!

Unfortunately, no existing test fails, showing wrong expectations, which means that it's entirely unclear what this change does. Since the diff-code is confusing enough as it is, any change must be accompanied by a test that protects it from regression and makes clear what the purpose of the change is.

Of course I understand that producing a test that fails without this change, ideally pushed in a commit before this one, is additional work, and it's understandable this request might go beyond what you can provide. If so, please feel free to close this PR, but otherwise, please do take your time, it's appreciated.

Copy link

ContributorAuthor

kamilkrzyskow commentedMay 28, 2024•
edited
Loading

OK, I expected this turn of events 😅, I'll see what I can do over the week.

EDIT:
I see that you allow for testing many things in one test function:

GitPython/test/test_diff.py

Lines 102 to 128 in14066e2

	deftest_diff_with_rename(self):
	output=StringProcessAdapter(fixture("diff_rename"))
	diffs=Diff._index_from_patch_format(self.rorepo,output)
	self._assert_diff_format(diffs)

	self.assertEqual(1,len(diffs))

	diff=diffs[0]
	self.assertTrue(diff.renamed_file)
	self.assertTrue(diff.renamed)
	self.assertEqual(diff.rename_from,"Jérôme")
	self.assertEqual(diff.rename_to,"müller")
	self.assertEqual(diff.raw_rename_from,b"J\xc3\xa9r\xc3\xb4me")
	self.assertEqual(diff.raw_rename_to,b"m\xc3\xbcller")
	assertisinstance(str(diff),str)

	output=StringProcessAdapter(to_raw(fixture("diff_rename_raw")))
	diffs=Diff._index_from_raw_format(self.rorepo,output)
	self.assertEqual(len(diffs),1)
	diff=diffs[0]
	self.assertIsNotNone(diff.renamed_file)
	self.assertIsNotNone(diff.renamed)
	self.assertEqual(diff.rename_from,"this")
	self.assertEqual(diff.rename_to,"that")
	self.assertEqual(diff.change_type,"R")
	self.assertEqual(diff.score,100)
	self.assertEqual(len(list(diffs.iter_change_type("R"))),1)

I'll hijack this loose rule and just add a test for lack of warnings when running theiter_change_type inside those functions 👌

EDIT2:

Or in this filehttps://github.com/gitpython-developers/GitPython/blob/main/test/deprecation/test_basic.py 👌

Copy link

Member

Byron commentedMay 28, 2024

Thanks a lot, and please do!

And maybe after doing that, putting matters into their own test won't be hard anymore, but if not, also no problem at all.

kamilkrzyskowand others added2 commits

June 8, 2024 01:30

Add deprecation test for DiffIndex.iter_change_type

d597088

Fix iter_change_type diff renamed property to prevent warning

e1c660d

kamilkrzyskow force-pushed thepatch-1 branch froma6977b7 toe1c660dCompare

June 8, 2024 00:19

Copy link

ContributorAuthor

kamilkrzyskow commentedJun 8, 2024

There you go, I added the test to the other deprecation tests intest_basic
I hope this will get released soon ;]

kamilkrzyskow requested a review fromByron

June 8, 2024 00:36

Byron approved these changes

Jun 8, 2024

View reviewed changes

Copy link

Member