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.

Sign up for GitHub

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

Jump to bottom

fix recursive call to _substmerge (issue #60) #64

Merged
merged 7 commits into from
Nov 29, 2023
Merged

fix recursive call to _substmerge (issue #60) #64

merged 7 commits into from
Nov 29, 2023

Conversation

zerwes
Copy link
Owner

@zerwes zerwes commented Nov 28, 2023

issue #60

@zerwes zerwes added the bug label Nov 28, 2023
@zerwes zerwes self-assigned this Nov 28, 2023
@zerwes zerwes mentioned this pull request Nov 28, 2023
Closed
reddart
reddart reviewed Nov 28, 2023
View reviewed changes
hiyapyco/__init__.py
for bd in b:
if isinstance(bd, dict):
a = self._deepmerge(a, bd)
a = self._substmerge(a, bd)
Copy link

Choose a reason for hiding this comment

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

@zerwes The Same change is also needed in line 366 and that should be it.

reddart
reddart approved these changes Nov 29, 2023
View reviewed changes
Copy link

@reddart reddart 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 lot @zerwes for taking the time. Looks good to me. Tested with my use cases.

@zerwes
Copy link
Owner Author

zerwes commented Nov 29, 2023

Hello again @reddart
Well, I hope I got it all sorted.
As the PR #37 came without some test I had implemented some after the merge.
But as the PR included some errors, the PR #38 trying to fix these, resulted in failing tests.
I inspected the failing tests, and indeed they where wrong in terms of the consequent implementation of substmerge.
IMHO now all should work as expected.
Please have a look at the changes in hiyapyco/__init__.pyand test/test_merge.py.
If you are fine with them, the merge can be completed.
Thank you for bringing this up again and helping clearing it.

@zerwes zerwes merged commit 724389b into main Nov 29, 2023
10 checks passed
@zerwes
Copy link
Owner Author

zerwes commented Nov 29, 2023

Thank you @reddart ; a new release will be out soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reddart reddart reddart approved these changes

Assignees

@zerwes zerwes

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zerwes @reddart