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: Merge config files on Config.set() #2214

Merged
merged 2 commits into from
Jul 11, 2016
Merged

fix: Merge config files on Config.set() #2214

merged 2 commits into from
Jul 11, 2016

Conversation

gnail
Copy link
Contributor

@gnail gnail commented Jun 29, 2016

The previous implementation overwrote properties so for sub-properties such as Config.client.* they could be overwritten if it's only partially specified.

Works with karma-runner/grunt-karma#194 to solve karma-runner/grunt-karma#165 and other issues mentioned in that PR.

@dignifiedquire
Copy link
Member

Thanks. Can you please add a test showing the changed behaviour?

@gnail
Copy link
Contributor Author

gnail commented Jun 29, 2016

Ok I've added a test for the change. The banner is showing the wrong info for some reason though - these are the correct Travis and AppVeyor builds. And it changed when I refreshed the page.

@dignifiedquire
Copy link
Member

Looks good thanks for this. Can you squash all the commits into one please and make the commit message so it follows our convention then I can merge it thanks :)

gnail added 2 commits July 10, 2016 16:42
The old behaviour will override child option nodes like options.client, causing properties to be lost if the new
option node is only partially specified. This partially causes karma-runner/grunt-karma#165 and
karma-runner/grunt-karma#166.
@gnail
Copy link
Contributor Author

gnail commented Jul 10, 2016

Ok I've rolled it into one commit (not including the rebase one). Do I need to include the rebase in there?

@dignifiedquire
Copy link
Member

Thanks :octocat:

@dignifiedquire dignifiedquire merged commit 1e526e3 into karma-runner:master Jul 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants