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

Issue 95 #143

Merged
merged 3 commits into from
May 1, 2018
Merged

Issue 95 #143

merged 3 commits into from
May 1, 2018

Conversation

aalba6675
Copy link
Contributor

config2.with_fallback(config1) has the unexpected side-effect of returning config1 and mutating it. The substitutions in config may also get mutated during the merge.

Fix this by doing a deepcopy of config1, config2

The tests

  • ensure that the return value is a new object different from config1
  • check that the ConfigValue in config2 has not been mutated

Addresses #95, #105

@coveralls
Copy link

coveralls commented Jan 12, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.3%) to 98.688% when pulling f036683 on aalba6675:issue-95 into 5682384 on chimpler:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 98.688% when pulling f036683 on aalba6675:issue-95 into 5682384 on chimpler:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 98.688% when pulling f036683 on aalba6675:issue-95 into 5682384 on chimpler:master.

@coveralls
Copy link

coveralls commented Jan 12, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.3%) to 98.688% when pulling f036683 on aalba6675:issue-95 into 5682384 on chimpler:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 98.688% when pulling 2204981 on aalba6675:issue-95 into 5682384 on chimpler:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jan 15, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.3%) to 98.688% when pulling 2204981 on aalba6675:issue-95 into 5682384 on chimpler:master.

darthbear
darthbear approved these changes May 1, 2018
View reviewed changes
Copy link
Member

@darthbear darthbear left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for taking care of it @aalba6675

@darthbear darthbear merged commit dd5d7d3 into chimpler:master May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darthbear darthbear darthbear approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aalba6675 @coveralls @darthbear