Skip to content
This repository has been archived by the owner on Sep 13, 2021. It is now read-only.

Bug/securetoken #71

Closed
wants to merge 4 commits into from
Closed

Bug/securetoken #71

wants to merge 4 commits into from

Conversation

nachiket87
Copy link

@nachiket87 nachiket87 commented Nov 28, 2020

What type of PR is this? (check all applicable)

  • Refactor
  • [x ] Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Fixed issue [#39 ]

Added to documentation?

  • readme
  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

![alt_text](gif url)
Image of Fix

@nachiket87 nachiket87 requested a review from a team as a code owner November 28, 2020 06:41
@nachiket87
Copy link
Author

Are there any reviewers?

@clairefro
Copy link
Contributor

Are there any reviewers?

Hi! Thanks again for this feature fix.

We haven't forgotten this and will get to it soon. Apologies that we've been internally backlogged and it's taken us some time to review.

Copy link
Contributor

@KmBarnett KmBarnett left a comment

Choose a reason for hiding this comment

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

The code is mostly good it just needs to be restructured. There are a few that need to be undone or explained.

@@ -15,7 +16,7 @@ export const config = (cmdObj) => {
console.log(chalk.cyanBright("Your current config:"));
console.log(`github username: ${getConfig("github")}`);
console.log(
`token: ${getConfig("token") ? "<hidden>" : "undefined"}`
`token: ${getConfig("token") ? `<hidden>` : "undefined"}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason quotes had to be changed? For consistency purposes, if they didn't need to be changed, revert this back.

@@ -44,7 +44,7 @@ export const createProgram = (args) => {
.description("Set up or view config (Github credentials etc.)")
.option("-v, --view", "view current config")
.option("-g, --github <username>", "set github username")
.option("-t, --token <token>", "set github personal access token")
.option("-t, --token", "set github personal access token")
Copy link
Contributor

Choose a reason for hiding this comment

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

<token> made the token a required argument for this command; Why did you remove it? Setting a Github token to undefined and getting the message Successfully set GitHub personal access token. is a bad flow.

@@ -18,6 +18,14 @@ const ghUsernameRegex = new RegExp(
"i"
);

const TOKEN = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is for the creation of new projects not for adding to the config. Lines 21-28 and lines 112-119 need to be extracted to a new file

@nachiket87
Copy link
Author

Accidentally request review again. Please ignore.

@nachiket87 nachiket87 closed this Jan 4, 2021
@nachiket87
Copy link
Author

I currently have an unsolvable node / yarn issue in running the cli on dev. I don't think I can fix this. Thanks for the time.

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

Successfully merging this pull request may close these issues.

3 participants