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.

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

Fix for issue 176: extends with increments can result in missing values #226

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

distributist
Copy link

This is a suggested fix for issue 176. I've added a test case based on the one presented in that issue, but have expanded it to test various combinations of operations.

The basic approach is to parse all the extended files before applying them, so that nothing gets lost along the way. Additionally, all annotations are kept so that even if a = overrides previous settings, the operation is captured so the problem can more easily be found.

I believe the fix behaves consistently, so that =, -= and += behave as expected within the order of extends operations. As the test case shows, you can do some stupid things, but it does them consistently. ;-)

@djay
Copy link

djay commented Jan 7, 2015

how does this relate to the existing fix in #177 ?

@distributist
Copy link
Author

Umm, I hadn't noticed that one. blush

I spent forever trying to figure out why a complex buildout was broken, finally found issue #176 which showed me why, and immediately set to work developing a patch I could deploy to move a project forward.

Looking at #177, the approach is different. I have changed update_section minimally, and instead use the recursive call of _open to return a flattened set of each config file, which then all get applied in order at the very end.

The test case tests several operations (+=, -=, =) at various levels, though I didn't test for the two items mentioned in #177: omitting the space before the += or having = and += in the same file. I'd be happy to do those if anyone thinks this fix has merit.

@distributist
Copy link
Author

I've just tried buildout 2.5.3 and it seems that this issue is still a problem. We have several complex buildouts and I've been patching them to work around this, but given that there are two fixes out there (mine and #177), is there a reason neither has been accepted?

I replied to the questions asked of me but there's been no response. As I stated, I'd be happy to test and incorporate the issues mentioned in #177. It would really be nice to fix this, and I'm willing to do so, but would like to know that someone with write access will look at this. If there are specific issues with my suggested fix, please let me know.

Thanks,
Hugh

@reinout
Copy link
Contributor

reinout commented Dec 6, 2016

I don't use the minus stuff myself (I do use plus). So I'm not familiar with the problems. A long while ago I added the plus-minus-issues label to related issues.

Would accepting this PR help fix the others? Or are there incompatible wishes in the various PRs? As said, I haven't got the full picture about what people are doing with these options.

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.

None yet

3 participants