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

Duplicate entries when adding a contributor twice #62

Closed
GantMan opened this issue Nov 8, 2017 · 13 comments · Fixed by #64
Closed

Duplicate entries when adding a contributor twice #62

GantMan opened this issue Nov 8, 2017 · 13 comments · Fixed by #64

Comments

@GantMan
Copy link
Contributor

GantMan commented Nov 8, 2017

if I add the same user with the same contributions, shouldn't it just no op?

And then if I add an existing user with a new contribution, shouldn't it just modify that contributor's contributions array? As far as for a CLI, it should be smart about the existing contents of the .all-contributorsrc correct?

@kentcdodds
Copy link
Collaborator

If I'm not mistaken, that's what it does. If it doesn't do that already, then that's a bug.

@GantMan
Copy link
Contributor Author

GantMan commented Nov 8, 2017

Mark as bug v4.7.0
idempotent

@machour machour changed the title Shouldn't the CLI be idempotent? Duplicate entries when adding a contributor twice Nov 8, 2017
@machour
Copy link
Collaborator

machour commented Nov 8, 2017

@GantMan tried with 4.7.0, I can't reproduce what you're describing:

# no differences on start

🍺  ~/git-point (master)*$ git diff .all-contributorsrc
🍺  ~/git-point (master)*$ ./node_modules/.bin/all-contributors add foo doc

# foo correctly added

🍺  ~/git-point (master)*$ git diff .all-contributorsrc
diff --git a/.all-contributorsrc b/.all-contributorsrc
index 6e89eaa..5bb0bbd 100644
--- a/.all-contributorsrc
+++ b/.all-contributorsrc
@@ -628,6 +628,15 @@
       "contributions": [
         "translation"
       ]
+    },
+    {
+      "login": "foo",
+      "name": "Foobar",
+      "avatar_url": "https://avatars0.githubusercontent.com/u/33384?v=4",
+      "profile": "http://dblp2.uni-trier.de/pers/hd/p/Foo:bar",
+      "contributions": [
+        "doc"
+      ]
     }
   ]
 }
🍺  ~/git-point (master)*$ ./node_modules/.bin/all-contributors add foo doc
🍺  ~/git-point (master)*$ ./node_modules/.bin/all-contributors add foo doc
🍺  ~/git-point (master)*$ ./node_modules/.bin/all-contributors add foo doc
🍺  ~/git-point (master)*$ ./node_modules/.bin/all-contributors add foo doc

# No duplication

🍺  ~/git-point (master)*$ git diff .all-contributorsrc
diff --git a/.all-contributorsrc b/.all-contributorsrc
index 6e89eaa..5bb0bbd 100644
--- a/.all-contributorsrc
+++ b/.all-contributorsrc
@@ -628,6 +628,15 @@
       "contributions": [
         "translation"
       ]
+    },
+    {
+      "login": "foo",
+      "name": "Foo Bar",
+      "avatar_url": "https://avatars0.githubusercontent.com/u/33384?v=4",
+      "profile": "http://dblp2.uni-trier.de/pers/hd/p/Foo:bar",
+      "contributions": [
+        "doc"
+      ]
     }
   ]
 }
🍺  ~/git-point (master)*$ ./node_modules/.bin/all-contributors add foo doc,code

# doc kept, code added when specifying old and new contribution

🍺  ~/git-point (master)*$ git diff .all-contributorsrc
diff --git a/.all-contributorsrc b/.all-contributorsrc
index 6e89eaa..a5053b1 100644
--- a/.all-contributorsrc
+++ b/.all-contributorsrc
@@ -628,6 +628,16 @@
       "contributions": [
         "translation"
       ]
+    },
+    {
+      "login": "foo",
+      "name": "Foo Bar",
+      "avatar_url": "https://avatars0.githubusercontent.com/u/33384?v=4",
+      "profile": "http://dblp2.uni-trier.de/pers/hd/p/Foo:bar",
+      "contributions": [
+        "doc",
+        "code"
+      ]
     }
   ]
 }

# doc & code kept, and bugs added when specifying only the new contribution

🍺  ~/git-point (master)*$ ./node_modules/.bin/all-contributors add foo bugs
🍺  ~/git-point (master)*$ git diff .all-contributorsrc
diff --git a/.all-contributorsrc b/.all-contributorsrc
index 6e89eaa..3d38887 100644
--- a/.all-contributorsrc
+++ b/.all-contributorsrc
@@ -628,6 +628,17 @@
       "contributions": [
         "translation"
       ]
+    },
+    {
+      "login": "foo",
+      "name": "Foo Bar",
+      "avatar_url": "https://avatars0.githubusercontent.com/u/33384?v=4",
+      "profile": "http://dblp2.uni-trier.de/pers/hd/p/Foo:bar",
+      "contributions": [
+        "doc",
+        "code",
+        "bugs"
+      ]
     }
   ]
 }

Can you provide the same test as me, adding a new user?

@GantMan
Copy link
Contributor Author

GantMan commented Nov 8, 2017

Yup! Fortunately, the whole thing is open source.

I created a ticket to add all-contributors-cli here: infinitered/solidarity#114

It was provided in a PR here: infinitered/solidarity#119
Investigating his PR, I tested it out and found the issue. All the code is available here: https://github.com/mattathompson/solidarity/tree/all-contributors

You can pull his branch and reproduce.

One thing worth noting, is that the changes are not meant for the readme.md, they are stored in a separate MD. This might be the difference.

Also 🎉 on the fast responses to this ticket! 👏

@machour
Copy link
Collaborator

machour commented Nov 8, 2017

@GantMan the project you're pointing at is using version 4.5.3. There were some changes that may have fixed this problem since then. Could you try with the latest version? (4.7.0 released an hour ago)

And thank you 😊 🎉

@GantMan
Copy link
Contributor Author

GantMan commented Nov 8, 2017

@machour - my global is 4.7.0, I just checked.

I also, started a fresh branch on master and tried it out: ❌
image

@machour
Copy link
Collaborator

machour commented Nov 8, 2017

which all-contributors from ~/Documents/Projects/playground/solidarity/ please.
I think that you are using the local (outdated) package here.

@GantMan
Copy link
Contributor Author

GantMan commented Nov 8, 2017

image

I also updated the package locally, and it was nope. Does this work on your machine? Are you using 4.7 from npm or a npm link version? I'm stuck on how I can figure it out :(

@kentcdodds
Copy link
Collaborator

Hmmm... We don't recommend installing this globally. And sadly we don't currently support a --version flag 😅

@machour
Copy link
Collaborator

machour commented Nov 8, 2017

@GantMan I'm using 4.7.0 from npm on gitpoint/git-point, as well as ./cli.js directly in this repository. Both work just fine.

As Kent mentioned, I think it would be better to get rid of your global package.

Then you may need to add some console.log() commands around here in your local package:
https://github.com/jfmengels/all-contributors-cli/blob/master/lib/contributors/add.js#L44
to understand what's going on.

Let us know what you find out 🕵️

@kentcdodds
Copy link
Collaborator

Are you sure that your global version of all-contributors is the latest version? I suggest you uninstall it globally and just use the local installation. ./node_modules/.bin/all-contributors should do it, or you might use npx, yarn, or this method to run binaries of locally install packages.

@GantMan
Copy link
Contributor Author

GantMan commented Nov 8, 2017

Verified version 4.7 (also removed global)
image
Page 2
image

Machine specs:
image
No global
image

@GantMan
Copy link
Contributor Author

GantMan commented Nov 8, 2017

I broke the code open and found the bug. PR coming soon hopefully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants