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

Add a build script to add contributors to the README #100

Merged
merged 6 commits into from
Oct 5, 2017
Merged

Add a build script to add contributors to the README #100

merged 6 commits into from
Oct 5, 2017

Conversation

yeskunall
Copy link
Contributor

@yeskunall yeskunall commented Oct 3, 2017

So I quickly put this together, because I want feedback before I make any drastic additions. Currently, it just adds a raw output (formatted to Markdown) at the end of the README, which works fine if it's the first time. But the next time it's run, it just appends the result to the end of the file, which isn't what we're looking for.

So my question is this: since @jakedex wants to add the contributors to the end of the README for now (as discussed in #92, should we just manually add them from time to time? When time comes, we can use something like all-contributors-cli to generate a CONTRIBUTORS.md for us and it will (hopefully) handle the issues I've pointed out here.

... or should I add some logic to the current script to check for existing users, and only add those that weren't previously in the list?

Thoughts?

@jakedex
Copy link
Collaborator

jakedex commented Oct 3, 2017

Thanks for getting the ball rolling on this. It looks like all-contributors-cli will handle injecting updates to the README.md, let's just add a .all-contributorsrc, a npm script for contrib:add, and contrib:build to precommit?

@yeskunall
Copy link
Contributor Author

Whoopsie, should have read their README a bit more closely. 🙊 if that's the case, I'll look into it right away!

@yeskunall
Copy link
Contributor Author

Hey @jakedex 👋

I just wanna make sure we're on the same page: you want a contrib:add that basically calls all-contributors add, which would mean a contributor is added using the CLI? How would that work?

... and I'm guessing contrib:build was to be followed by the previous command that ran all-contributors generate?

In any case, look at the changes I've introduced. It works just fine, except that there's no way to add emoji keys to the contributors as I'm just pulling in the contributors, not the type of contribution (which I don't think is possible)

@mfix22
Copy link
Contributor

mfix22 commented Oct 4, 2017

@yeskunall thanks for this! Now that we are going with all-contributors, you can just use all-contributors add and all-contributors generate. See these lines for reference.

And actually, the emojis are generated from the key words in the contributions array. You can see an example here: https://github.com/kentcdodds/all-contributors/blob/master/.all-contributorsrc#L15-L19

@jakedex
Copy link
Collaborator

jakedex commented Oct 4, 2017

Apologies for the vague response, @yeskunall. I was trying to say that we should just do this all manually with the npm scripts and scrap contributors.js for the time being. Again, sorry for the confusion!

@yeskunall
Copy link
Contributor Author

yeskunall commented Oct 4, 2017

All right, understood. I'll scrap the script and manually add the contributors and update the README. Gotta leave for work, but this should be merged by today evening.

❤️

@yeskunall
Copy link
Contributor Author

All right, so the npm scripts are done and good to go. Final question: @mfix22, who's going to add the emojis keys to the contributors' block? 😛

I'm willing to do it, but the CLI asks questions of the likes:

capture

... and I'm not sure if I should be the one deciding that. 🚶

(Once I get a response, I'll also fix the npm precommit script. Noticed it after I pushed 🙈

@mfix22
Copy link
Contributor

mfix22 commented Oct 5, 2017

I can handle that @yeskunall (: mind if i run the CLI and push to your branch?

@mfix22 mfix22 self-assigned this Oct 5, 2017
@yeskunall
Copy link
Contributor Author

yeskunall commented Oct 5, 2017 via email

Copy link
Contributor Author

@yeskunall yeskunall left a comment

Choose a reason for hiding this comment

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

LGTM. We ready to merge?

@mfix22 mfix22 changed the title [WIP] Add a build script to add contributors to the README Add a build script to add contributors to the README Oct 5, 2017
@mfix22
Copy link
Contributor

mfix22 commented Oct 5, 2017

Thanks @yeskunall for all your work and comments!

@mfix22 mfix22 merged commit 4bc3353 into carbon-app:master Oct 5, 2017
@yeskunall yeskunall deleted the feat-issue-79 branch October 5, 2017 03:23
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.

3 participants