Skip to content

Proposed change to get_values behavior - #1535

Closed
kfreezen wants to merge 1 commit into
gitpython-developers:mainfrom
kfreezen:fix/get_values
Closed

Proposed change to get_values behavior#1535
kfreezen wants to merge 1 commit into
gitpython-developers:mainfrom
kfreezen:fix/get_values

Conversation

@kfreezen

Copy link
Copy Markdown
Contributor

get_values() did not load existing sections before checking the _sections property.

NOTE: Haven't written a test to check for this yet.

@kfreezen

Copy link
Copy Markdown
ContributorAuthor

Proposed fix for #1534

@kfreezen

Copy link
Copy Markdown
ContributorAuthor

Looking at test here; I suppose we can leave it raising a KeyError. The main fix was force-loading the _sections attribute before checking the list.

I'll re-commit using KeyError.

@ByronByron added this to the v3.1.31 - Bugfixes milestone Jan 11, 2023

@ByronByron left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot, much appreciated!

I think the commit should link back to the issue it resolve to create a more declarative history.
Finally it should be possible to devise a test that would fail without this change to cement it. Otherwise it might be removed in future because it's unclear why eager loading of sections is necessary.

@Codym48

Copy link
Copy Markdown
Contributor

@kfreezen, consider accepting kfreezen#1 to add test coverage for this change? I verified it fails on main:

image

@Byron

Copy link
Copy Markdown
Member

Closing as it's superseded by #1555

@ByronByron closed this Feb 16, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@kfreezen@Codym48@Byron