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 not being able to remove networks from user config #233

Merged
merged 1 commit into from Apr 2, 2016

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Apr 2, 2016

Fixes #211, a bug introduced by #57.

Previously it used to simply override entire networks object, but #57 made it use merge method, which makes it impossible to remove disconnected networks. This PR changes it to assign which works similarly to merge, but only on root keys, which overrides the networks object just fine.

http://stackoverflow.com/a/33247597/2200891

@xPaw xPaw added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Apr 2, 2016
@AlMcKinlay
Copy link
Member

So, am I right in saying that if we are removing a network, the opts object will have that network as undefined, and "assign" will then override it to be undefined in the user object, but "merge" will always set it to the non-undefined one, which means it can't be removed?

@AlMcKinlay AlMcKinlay self-assigned this Apr 2, 2016
@xPaw
Copy link
Member Author

xPaw commented Apr 2, 2016

Not sure I understand, in which case would it be undefined? The network is removed using _.without. And opts is always passed in with networks object.

https://github.com/thelounge/lounge/blob/2201e2b941bd13427c38d6934b84ceda245aa20e/src/plugins/inputs/quit.js#L10
https://github.com/thelounge/lounge/blob/2201e2b941bd13427c38d6934b84ceda245aa20e/src/client.js#L411-L413

@AlMcKinlay
Copy link
Member

Well I don't understand the change then. What is assign doing that merge doesn't then?

@xPaw
Copy link
Member Author

xPaw commented Apr 2, 2016

It only overwrites root keys, so the entire networks object is overwritten.

@AlMcKinlay
Copy link
Member

Right, that's what I meant. So the new networks object has the old networks without the one(s) that was removed. If merge was being used, it would take the non-undefined networks from the user object and use them in the merge, yeah?

@xPaw
Copy link
Member Author

xPaw commented Apr 2, 2016

Yes.

@AlMcKinlay
Copy link
Member

Cool. 👍 . Simple fix, so no need for second review. Merging.

@AlMcKinlay AlMcKinlay merged commit bb00d42 into master Apr 2, 2016
@AlMcKinlay AlMcKinlay deleted the xpaw/fix-config branch April 2, 2016 15:32
@astorije astorije added this to the 1.4.3 milestone Oct 8, 2016
@AlMcKinlay AlMcKinlay removed their assignment Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants