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

Test for issue #32 #33

Merged
merged 2 commits into from Jun 10, 2017
Merged

Conversation

nkitchen
Copy link
Contributor

@nkitchen nkitchen commented Jun 9, 2017

This test file contains the case I posted in the issue thread.

@awalterschulze
Copy link
Owner

awalterschulze commented Jun 9, 2017 via email

@awalterschulze
Copy link
Owner

Do you want to contribute this fix in the pull request as well?

diff --git a/attrs.go b/attrs.go
index a00eeb7..5e1518a 100644
--- a/attrs.go
+++ b/attrs.go
@@ -56,9 +56,7 @@ func (attrs Attrs) Extend(more Attrs) {
 // Ammend only adds the missing attributes to attrs Attrs type.
 func (attrs Attrs) Ammend(more Attrs) {
        for key, value := range more {
-               if _, ok := attrs[key]; !ok {
-                       attrs.add(key, value)
-               }
+               attrs.add(key, value)
        }
 }

@nkitchen
Copy link
Contributor Author

nkitchen commented Jun 9, 2017

Your suggested fix just makes Ammend the same as Extend. Is that really what you intended?

@awalterschulze
Copy link
Owner

Good spot, yes maybe I just want to rather call extend and remove the ammend method. I seems that ammend is not really what you ever want to do.

@nkitchen
Copy link
Contributor Author

Yes, I came to basically the same conclusion after adding several more test cases. In fact, the proposed fix that I will send includes changing Nodes.Add to call Extend.

@awalterschulze
Copy link
Owner

Great work :)
I think this is ready to merge and you?

@nkitchen
Copy link
Contributor Author

Yes, I believe this completely resolves the issue.

@awalterschulze awalterschulze merged commit 67d8d2b into awalterschulze:master Jun 10, 2017
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.

None yet

2 participants