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 issue #71 -Wrong contribution type being added to .all-contributorsrc #78

Merged
merged 19 commits into from
Jun 8, 2018

Conversation

M-ZubairAhmed
Copy link
Contributor

@M-ZubairAhmed M-ZubairAhmed commented Dec 23, 2017

What:
Issue #71

Why:
Wrong contribution type being added to .all-contributorsrc despite the error

How:

  • I have included that change in prompt.js
  • I am checking if its a valid type from utils
  • Currently i am passing the filtered types further down the program . eg
    if user types add foo code,bar,video
  • code and video are added
    and if user types all wrong types then nothing will be added with an error (please tell me if this is what you are looking for)?

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@codecov-io
Copy link

codecov-io commented Dec 23, 2017

Codecov Report

Merging #78 into master will increase coverage by 3.82%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   53.99%   57.81%   +3.82%     
==========================================
  Files          19       19              
  Lines         413      422       +9     
  Branches       68       70       +2     
==========================================
+ Hits          223      244      +21     
+ Misses        161      149      -12     
  Partials       29       29
Impacted Files Coverage Δ
src/generate/format-contribution-type.js 100% <ø> (ø) ⬆️
src/contributors/prompt.js 61.76% <100%> (+61.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4a0484...e51c5a5. Read the comment docs.

Copy link
Collaborator

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This is looking great! Do you think you could add a test?

@M-ZubairAhmed
Copy link
Contributor Author

Yes I can add

@M-ZubairAhmed
Copy link
Contributor Author

should I make a new file under test folder for prompt, since that's where I have inserted the code

@kentcdodds
Copy link
Collaborator

Either way

@M-ZubairAhmed M-ZubairAhmed changed the title initial issue fix fix issue #71 -Wrong contribution type being added to .all-contributorsrc Dec 25, 2017
@M-ZubairAhmed
Copy link
Contributor Author

M-ZubairAhmed commented Dec 25, 2017

this also checks off following from issue #6

  • Contribution type not found.
  • No contribution types when adding a contributor.

Copy link
Collaborator

@machour machour left a comment

Choose a reason for hiding this comment

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

Hi @M-ZubairAhmed and thank you for this PR! 🎉

Left a few comments here and there. I think you broke the prompt.

Also, could you keep your PR focused on #71 and avoid changing other issues related stuff? It would be easier to review and accept smaller contributions.

Last commit for example, should be reverted.

const contributions = ''
expect(() => prompt(options, username, contributions)).toThrow(
`No contribution type found in the input. Did you forget to include them in the add command?`,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If no contribution types are found in the add command, the user should be prompted for them.

I should be able to just run the cli without any argument, be prompted for the operation I want to do (add/generate), then be prompted for the rest of the arguments.

Currently your patch breaks that:

screen shot 2017-12-25 at 12 20 13 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok let us handle this in another pr then

README.md Outdated
- [Configuration](#configuration)
- [Inspiration](#inspiration)
- [Other Solutions](#other-solutions)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be dealt with in another PR.

README.md Outdated
@@ -167,13 +168,11 @@ Thanks goes to these wonderful people
([emoji key](https://github.com/kentcdodds/all-contributors#emoji-key)):

<!-- ALL-CONTRIBUTORS-LIST:START - Do not remove or modify this section -->

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you remove that space by hand, or did the command output change? 🤔

@M-ZubairAhmed
Copy link
Contributor Author

i have reverted all the change to unwanted files

@M-ZubairAhmed
Copy link
Contributor Author

currently the toc is being generated when i commit a readme file automatically

@M-ZubairAhmed
Copy link
Contributor Author

I have added my self as contributor in this #79 pr

@machour
Copy link
Collaborator

machour commented Dec 25, 2017

Thank you for the rollbacks :)

Tested the new PR, I still see it breaking things:

🍺  ~/all-contributors-cli (master)*$ ./src/cli.js
? What do you want to do? Add a new contributor or add a new contribution type
Invalid contribution type/s entered

Normally here, I would have been asked first for the login, then for the contributions types (with a nice selection interface). Your PR seems to force the user to give all arguments on the command line. (did you know about this?)

@M-ZubairAhmed
Copy link
Contributor Author

I did not know about this as in the issue it was mentioned error has occured because of

~/git-point (master)*$ yarn contributors:add foo bar

So i tested the code with that command and did not look at the CLI. What would you like me to do?

@M-ZubairAhmed
Copy link
Contributor Author

can you take a look at this now? Cli seems to be working. May be i am missing some edge cases of the usage

Copy link
Collaborator

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thanks!

@M-ZubairAhmed
Copy link
Contributor Author

any status on this :-)

@machour
Copy link
Collaborator

machour commented Dec 27, 2017

@M-ZubairAhmed busy week for me, I'll try to recheck this soon.

@M-ZubairAhmed
Copy link
Contributor Author

M-ZubairAhmed commented May 23, 2018

hi,
sorry couldnt get back on this earlier, @machour could you please check this

@M-ZubairAhmed
Copy link
Contributor Author

Hi,
Sorry any updates on this?

Copy link
Collaborator

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Just one thing, other than that, this is great!

)(userContributions)

if (_.isEmpty(validUserContributions)) {
throw new Error('Invalid contribution type(s) entered')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we make the error include a comma-separated list of the invalid contribution types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean to say, have a list of valid contribution types?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I want them to be able to see which contribution type they entered which was invalid so they know not to enter that again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added please take a look 👍

Copy link
Collaborator

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

@kentcdodds kentcdodds merged commit 526d4db into all-contributors:master Jun 8, 2018
@M-ZubairAhmed M-ZubairAhmed deleted the bug-wrgTypeAdded branch June 8, 2018 23:08
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.

4 participants