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

Overwrite clobber config fields #2062

Merged
merged 1 commit into from
Jan 27, 2020

Conversation

drewbanin
Copy link
Contributor

Fixes #2049 by updating the Source Config class to overwrite, instead of deep merge, keys marked as "clobberable".

@cla-bot cla-bot bot added the cla:yes label Jan 20, 2020
@drewbanin drewbanin changed the base branch from dev/0.15.1 to dev/0.15.2 January 20, 2020 23:44
@drewbanin drewbanin changed the base branch from dev/0.15.2 to dev/barbara-gittings January 20, 2020 23:44
@drewbanin drewbanin mentioned this pull request Jan 20, 2020
5 tasks
@drewbanin drewbanin changed the base branch from dev/barbara-gittings to dev/0.15.2 January 20, 2020 23:46
@drewbanin drewbanin force-pushed the fix/overwrite-clobber-config-fields branch from 5c5ae43 to ec56d4f Compare January 20, 2020 23:48
@drewbanin drewbanin marked this pull request as ready for review January 20, 2020 23:53
@drewbanin drewbanin requested a review from beckjake January 20, 2020 23:53
Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

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

LGTM, one tiny comment

intermediary_merged = deep_merge(
merged_config.copy(), config.copy()
merged_config.copy(), config
Copy link
Contributor

Choose a reason for hiding this comment

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

This .copy() shouldn't be necessary, deep_merge calls deepcopy.

@drewbanin drewbanin changed the base branch from dev/0.15.2 to dev/barbara-gittings January 24, 2020 20:21
@drewbanin drewbanin force-pushed the fix/overwrite-clobber-config-fields branch from 3e5e35d to 1f74980 Compare January 24, 2020 20:23
@@ -50,9 +50,16 @@ def __init__(self, active_project, own_project, fqn, node_type):
def _merge(self, *configs):
merged_config = {}
for config in configs:
# Do not attempt to deep merge clobber fields
Copy link
Contributor Author

@drewbanin drewbanin Jan 24, 2020

Choose a reason for hiding this comment

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

@beckjake I preserved the copy here since we pop elements out of the dict in the comprehension below. Can you give this another quick look?

Edit: also rebased this against dev/barbara-gittings

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think that's the correct behavior

@drewbanin drewbanin merged commit fdfcd4c into dev/barbara-gittings Jan 27, 2020
@drewbanin drewbanin deleted the fix/overwrite-clobber-config-fields branch January 27, 2020 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants