GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix iter_change_type diff renamed property to prevent warning by kamilkrzyskow · Pull Request #1918 · gitpython-developers/GitPython

Conversation

@kamilkrzyskow

@kamilkrzyskow kamilkrzyskow commented May 28, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

GitPython/git/diff.py

Lines 328 to 329 in 9fbfb71

elif change_type == "R" and diffidx.renamed:
yield diffidx

Given that the code wasn't reported yet, perhaps I'm doing something wrong using the iter_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 the iter_change_type has any tests.

Thanks for your time ✌️

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Byron
Byron requested changes May 28, 2024
View reviewed changes

@Byron Byron left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kamilkrzyskow

kamilkrzyskow commented May 28, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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 in 14066e2

def test_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")
assert isinstance(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 the iter_change_type inside those functions 👌

EDIT2:

Or in this file https://github.com/gitpython-developers/GitPython/blob/main/test/deprecation/test_basic.py 👌
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Byron

Byron commented May 28, 2024

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kamilkrzyskow and others added 2 commits June 8, 2024 01:30
@kamilkrzyskow
Add deprecation test for DiffIndex.iter_change_type
d597088
@kamilkrzyskow
Fix iter_change_type diff renamed property to prevent warning
e1c660d
@kamilkrzyskow
kamilkrzyskow force-pushed the patch-1 branch from a6977b7 to e1c660d Compare June 8, 2024 00:19
@kamilkrzyskow

kamilkrzyskow commented Jun 8, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kamilkrzyskow
kamilkrzyskow requested a review from Byron June 8, 2024 00:36
Byron
Byron approved these changes Jun 8, 2024
View reviewed changes

@Byron Byron left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

My apologies, I realize my mistake now! rename is deprecated and was used internally, even though renamed_file should be used.
I thought this change is a affecting the logic of the diff, even though it's not doing that at all.
🤦‍♂️

Oh well, thanks again for bearing with me here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kamilkrzyskow reacted with hooray emoji kamilkrzyskow reacted with rocket emoji
All reactions
  • 🎉 1 reaction
  • 🚀 1 reaction
Comment thread test/deprecation/test_basic.py

def test_diff_iter_change_type(diffs: "DiffIndex") -> None:
"""The internal DiffIndex.iter_change_type function issues no deprecation warning."""
with assert_no_deprecation_warning():

@Byron Byron Jun 8, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

There must have been a misunderstanding. It's not about deprecation warnings, it's about adding a test that fails without the change presented here, while making clear what the issue truly is.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Byron
Byron merged commit ee987da into gitpython-developers:main Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Byron Byron Byron approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@kamilkrzyskow @Byron
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.