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.

Sign up for GitHub

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

Jump to bottom

feat: display a meaningful error when the config file is missing #288

Merged
merged 7 commits into from Oct 4, 2020
Merged

feat: display a meaningful error when the config file is missing #288

merged 7 commits into from Oct 4, 2020

Conversation

darekkay
Copy link
Contributor

@darekkay darekkay commented Oct 3, 2020

What

When you try to add a user with .all-contributorsrc missing, you will get a cryptic error message (see also all-contributors/all-contributors#378):

$ all-contributors add darekkay maintenance
Cannot read property 'then' of null

This PR displays a meaningful error message in this case.

Why

So users know what's causing the problem.

How

Before a contribution is being added, the script checks whether the config file exists. If it doesn't, an error message is being displayed:

$ all-contributors add darekkay maintenance
Configuration file not found: C:\projects\test\.all-contributorsrc

Checklist:

  • Documentation N.A.
  • Tests (the cli doesn't have any tests)
  • Ready to be merged
  • Added myself to contributors table

@darekkay
feat: display a meaningful error when the config file is missing
26ddea2 
## What

When you try to add a user with `.all-contributorsrc` missing, you will get a cryptic error message (see also all-contributors/all-contributors#378):

```
$ all-contributors add darekkay maintenance
Cannot read property 'then' of null
```

This PR displays a meaningful error message in this case.

## Why

So users know what's causing the problem.

## How

Before a contribution is being added, the script checks whether the config file exists. If it doesn't, an error message is being displayed:

```
$ all-contributors add darekkay maintenance
Configuration file not found: C:\projects\test\.all-contributorsrc
```
@darekkay
docs: add darekkay as a contributor
984237e
Berkmann18
Berkmann18 requested changes Oct 3, 2020
View reviewed changes
Copy link
Member

@Berkmann18 Berkmann18 left a comment

Choose a reason for hiding this comment

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

Would it be possible to add/update tests?

@darekkay darekkay mentioned this pull request Oct 3, 2020
Merged
4 tasks
@darekkay
Copy link
Contributor Author

darekkay commented Oct 3, 2020

The ESlint warning is not caused by this PR. I've fixed it in a separate PR: #289

Would it be possible to add/update tests?

There are no tests for the whole cli.js file. And I get why - because testing addContribution on its own probably isn't easy. If a missing test for an untested file is blocking this PR from being merged, feel free to close it (or leave it open for someone else to pick up).

@Berkmann18
Copy link
Member

There are no tests for the whole cli.js file. And I get why - because testing addContribution on its own probably isn't easy. If a missing test for an untested file is blocking this PR from being merged, feel free to close it (or leave it open for someone else to pick up).

That's a good point CLI E2E tests can be very tricky to do.

1 similar comment
@Berkmann18
Copy link
Member

There are no tests for the whole cli.js file. And I get why - because testing addContribution on its own probably isn't easy. If a missing test for an untested file is blocking this PR from being merged, feel free to close it (or leave it open for someone else to pick up).

That's a good point CLI E2E tests can be very tricky to do.

@Berkmann18 Berkmann18 merged commit d517440 into all-contributors:master Oct 4, 2020
@darekkay darekkay deleted the feat/handle-add-error branch October 4, 2020 20:46
@all-contributors-release-bot
Copy link
Member

🎉 This PR is included in version 6.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

tenshiAMD added a commit that referenced this pull request Sep 13, 2022
@tenshiAMD
Merge remote-tracking branch 'origin/master' into feat/style-table
6744c99 
* origin/master: (85 commits)
  refactor: log full error stack on error (#316)
  chore: fix status badges (#315)
  docs: add JoshuaKGoldberg as a contributor for bug (#314)
  fix: incorrect usage of `tbody` (#311)
  fix: trim `nextLink` before slicing (#309)
  fix: set default value as `7` for `contributorsPerLine` (#139)
  chore(deps): bump dependencies and devDeps (#298)
  refactor: add tbody to contributors table (#307)
  docs: add Lucas-C as a contributor for doc (#306)
  fix: scriptName + improving usage messages (#305)
  docs: add vapurrmaid as a contributor (#304)
  chore(deps): CVE-2021-23337 in inquirer->lodash (#303)
  docs: add SirWindfield as a contributor (#297)
  feat: add namespaced token (#296)
  docs: add LaChapeliere as a contributor (#292)
  feat(contribution-types): add research contribution type (#291)
  docs: add darekkay as a contributor (#290)
  feat: display a meaningful error when the config file is missing (#288)
  docs: add melink14 as a contributor (#285)
  docs: add jdalrymple as a contributor (#264)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Berkmann18 Berkmann18 Berkmann18 approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@darekkay @Berkmann18 @all-contributors-release-bot