PR for Issue6: projectOwner & projectName is not set in .all-contributorsrc file. #80

M-ZubairAhmed · 2017-12-27T13:42:56Z

What:

b and c of Handle common errors #6

Why:

As no error message is in place to check if project name and owner are present

How:

Added check in config-file before config is written into a json file
Added check in check before url is being constructed
wrapped json parse method in try catch from check to handle error

Checklist:

Documentation
Tests
Ready to be merged
Added myself to contributors table

…init command

2. added check for owner and name in getcontributionsfromgithub before constructing url

codecov-io · 2017-12-27T13:45:45Z

Codecov Report

Merging #80 into master will increase coverage by 0.44%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
+ Coverage   53.54%   53.99%   +0.44%     
==========================================
  Files          19       19              
  Lines         409      413       +4     
  Branches       66       68       +2     
==========================================
+ Hits          219      223       +4     
  Misses        161      161              
  Partials       29       29

Impacted Files	Coverage Δ
src/util/config-file.js	`68.18% <100%> (+7.07%)`	⬆️

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 1562a37...fdcb6dd. Read the comment docs.

machour

Thank you @M-ZubairAhmed! Left you some comments.

Do you think you could add some tests for this?

machour · 2017-12-27T14:12:37Z

src/util/check.js

+        body = JSON.parse(res.body)
+        contributorsIds = body.map(contributor => contributor.login)
+      } catch (error) {
+        throw new Error(error)


Why do we catch if we're going to throw anyways? 🤔

oh ok should i use chalk to display the error

machour · 2017-12-27T14:13:08Z

src/util/check.js

-  return getContributorsPage(url)
+  if (owner === '') {
+    throw new Error('Error! Project owner is not set in .all-contributorsrc')
+  } else if (name === '') {


no need for else

ok, i will replace it with if

machour · 2017-12-27T14:13:29Z

src/util/check.js

+    throw new Error('Error! Project owner is not set in .all-contributorsrc')
+  } else if (name === '') {
+    throw new Error('Error! Project name is not set in .all-contributorsrc ')
+  } else {


no need for else here too

machour · 2017-12-27T14:14:05Z

src/util/config-file.js

+    throw new Error(`Error! Project Name is not set in ${configPath}`)
+  } else if (content.projectName === '') {
+    throw new Error(`Error! Project Name is not set in ${configPath}`)
+  } else {


same remarks for elses

M-ZubairAhmed · 2017-12-27T15:01:11Z

i did the changes, i will add the tests soon

kentcdodds

Looking good! Just one thing

kentcdodds · 2018-01-02T17:36:18Z

src/util/config-file.js

@@ -14,6 +14,12 @@ function readConfig(configPath) {
 }

 function writeConfig(configPath, content) {
+  if (content.projectOwner === '') {


Instead of '', why don't we do a simple falsy check? if (!content.projectOwner)

This has the added benefit of working if the value is set to null or undefined somehow.

We should do that for all the checks I think.

@kentcdodds thank you for your comment, i however feel that it could be a big pr, would you like me to add that in the current pr?

It's not an addition, it's simply a change to your addition. This change wont make the PR any bigger...

sure will get on it

i didnt knew this https://stackoverflow.com/a/8693015.
Thank you @kentcdodds

…null obj value

M-ZubairAhmed · 2018-05-23T12:02:54Z

added changes as per @kentcdodds comment. Please @machour @kentcdodds review when you guys get time

kentcdodds · 2018-05-23T13:06:05Z

src/util/check.js

+module.exports = function getContributorsFromGithub(owner, name) {
+  if (!owner) {
+    throw new Error(
+      `${owner} aaaaError! Project owner is not set in .all-contributorsrc`,


I'm guessing this aaaa here is a typo?

Other than that, this looks good!

damn how did i miss that. Fixed it

kentcdodds · 2018-05-24T15:53:35Z

src/util/check.js

@@ -0,0 +1,54 @@
+const pify = require('pify')


Sorry, it's been a little while, but why does this file exist? It doesn't appear to be in use anywhere in the project (other than tests)...

Sorry but this is a node package isn't it?

Sorry, I should have been more clear. I'm commenting on the file src/utils/check.js not pify.

sorry for being late.
It seems that this file was removed in this 152a1fe commit. I retested everything

M-ZubairAhmed · 2018-05-28T10:48:02Z

src/util/__tests__/config-file.js

@@ -1,15 +1,43 @@
 import configFile from '../config-file'

 const absentFile = './abc'
-const expected = `Configuration file not found: ${absentFile}`
+const absentConfileFileExpected = `Configuration file not found: ${absentFile}`


renamed this as, we have more test with expected in this file now.

kentcdodds

This looks good. Thanks so much!

* docs: update README.md * docs: update .all-contributorsrc

z added 4 commits December 27, 2017 17:04

error message when no project name and project owner is added in the …

f9e5597

…init command

moved error message away from init prompt

f95b879

1. placed json parse in try catch

e14510e

2. added check for owner and name in getcontributionsfromgithub before constructing url

added check before config file is written

4d54771

machour requested changes Dec 27, 2017

View reviewed changes

changes as per all-contributors#80 (review)

a3159d4

z added 3 commits December 27, 2017 20:34

uncommented code

e6172a9

tests added

a69fd48

Added myself to contributors table

5250432

kentcdodds reviewed Jan 2, 2018

View reviewed changes

M-ZubairAhmed added 3 commits May 23, 2018 06:07

merge latest to current branch

ac96910

test inside utils were moved to github in master branch

30d2faa

changes to validation check, instead of checking for empty string to …

862e982

…null obj value

kentcdodds reviewed May 23, 2018

View reviewed changes

typo fixed

dce26d8

kentcdodds reviewed May 24, 2018

View reviewed changes

unnecisary check.js file removed

fdcb6dd

M-ZubairAhmed commented May 28, 2018

View reviewed changes

kentcdodds approved these changes May 29, 2018

View reviewed changes

kentcdodds merged commit c4a0484 into all-contributors:master May 29, 2018

M-ZubairAhmed deleted the message-noOwner branch May 30, 2018 10:27

M-ZubairAhmed mentioned this pull request Feb 23, 2019

Handle common errors #6

Open

11 tasks

Berkmann18 pushed a commit that referenced this pull request May 24, 2020

docs: add protocols to profile URLs (#80)

68a84b0

* docs: update README.md * docs: update .all-contributorsrc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR for Issue6: projectOwner & projectName is not set in .all-contributorsrc file. #80

PR for Issue6: projectOwner & projectName is not set in .all-contributorsrc file. #80

M-ZubairAhmed commented Dec 27, 2017 •

edited

Loading

codecov-io commented Dec 27, 2017 •

edited

Loading

machour left a comment

machour Dec 27, 2017

M-ZubairAhmed Dec 27, 2017

machour Dec 27, 2017

M-ZubairAhmed Dec 27, 2017

machour Dec 27, 2017

machour Dec 27, 2017

M-ZubairAhmed commented Dec 27, 2017

kentcdodds left a comment

kentcdodds Jan 2, 2018

M-ZubairAhmed May 23, 2018

kentcdodds May 23, 2018

M-ZubairAhmed May 23, 2018

M-ZubairAhmed May 23, 2018

M-ZubairAhmed commented May 23, 2018

kentcdodds May 23, 2018

M-ZubairAhmed May 23, 2018

kentcdodds May 24, 2018

M-ZubairAhmed May 25, 2018

kentcdodds May 25, 2018

M-ZubairAhmed May 28, 2018

M-ZubairAhmed May 28, 2018

kentcdodds left a comment

PR for Issue6: projectOwner & projectName is not set in .all-contributorsrc file. #80

PR for Issue6: projectOwner & projectName is not set in .all-contributorsrc file. #80

Conversation

M-ZubairAhmed commented Dec 27, 2017 • edited Loading

codecov-io commented Dec 27, 2017 • edited Loading

Codecov Report

machour left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-ZubairAhmed commented Dec 27, 2017

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-ZubairAhmed commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

M-ZubairAhmed commented Dec 27, 2017 •

edited

Loading

codecov-io commented Dec 27, 2017 •

edited

Loading