Add private flag for yarn init command #4165

pierreneter · 2017-08-14T08:04:33Z

Summary
Close #4113, this PR create the private flag for yarn init command. Can use both yes and private flags. Or can use private flag alone to set private is true when run command.

Test plan
Have added tests

rally25rs · 2017-08-14T12:56:23Z

src/cli/commands/init.js

+    {
+      key: 'private',
+      question: 'private',
+      default: '',


Could also add validation and validationError fields to ensure the user enters something that "yn" can convert to boolean (or only accept "true" and "false" and eliminate the dependency on "yn").

I think 1 i shorter way user want to fill this key, so i use yn. And everything can be become a boolean when we use yn. So, if users want to change it, they can change it later by edit package.json file. Just boolean value is approved.

This is a valid point. I was thinking yn might error if the user types in something like "i guess so" as the answer, but I suspect it will just try to convert it anyway. As long as yn doesn't throw an exception, then I would be OK with no validation.

rally25rs · 2017-08-14T12:57:19Z

src/cli/commands/init.js

+    {
+      key: 'private',
+      question: 'private',
+      default: '',


If the default was default: defaultFlag ? 'true' : 'false' then it would eliminate a conditional below

If the above change is not done, the default should probably be "false" instead of empty string so that when the question is printed to the user they can see that it is explicitly false by default. Empty string might make someone question if the default is actually going to end up true or false (in reality it depends how "yn" interprets empty string)

I keep '' because we don't need add line private: {boolean} into package.json If users don't set value for private when they run: yarn init. And one thing, true and false are a boolean value, not a string, so I don't wrapper it in ''.

rally25rs · 2017-08-14T12:58:09Z

src/cli/commands/init.js

    if (def) {
-      question += ` (${def})`;
+      question += ` (${def.toString()})`;


Why the additional .toString()?

because after i add this flag, def could be string or possibly boolean. I want to make sure it will be a string when added to question.

But it's already inside a string interpolation, so would get converted to a string already.

If removed it will generate 2 errors:

$ eslint . && flow check Error: src/cli/commands/init.js:130 130: question += ` (${def})`; ^^^ boolean. This type cannot be coerced to 130: question += ` (${def})`; ^^^^^^^^^^^ string

That's interesting. That is Flow complaining about the type. I didn't think it would complain inside an interpolated string. Oh well, in that case the .toString() is fine.

rally25rs · 2017-08-14T12:58:44Z

src/cli/commands/init.js

@@ -114,14 +121,18 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
      def = val;
    }

+    if (manifestKey === 'private' && privateFlag) {


This conditional could be eliminated if the default is set above in the array of questions

no need, just add true when user uses this flag.

rally25rs · 2017-08-14T13:00:20Z

src/cli/commands/init.js

    }

    let answer;
    let validAnswer = false;

-    if (yes) {
+    if (yes || (privateFlag && manifestKey === 'private')) {


I think this additional || would also be eliminated if the default was set based ont he flag in the array of questions.

Like I said below. No default value.

rally25rs · 2017-08-14T13:03:14Z

src/cli/commands/init.js

@@ -141,6 +152,9 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
    }

    if (answer) {
+      if (manifestKey === 'private') {


It would be a better design to add something like dataType: 'boolean' to the schema of questions when they are defined, then here check that data type and use "yn" in the case of booleans only.
Or perhaps even inputFormatter: yn in the questions schema, then here: if( question.inputFormatter) { answer = question.inputFormatter(answer); }

rally25rs

Thank you for the contribution!

Added some comments that I thing would simplify the code. I dislike the "special case" if manifestKey === 'private' conditionals. It would be nice to eliminate those.

BYK

Nice!

BYK · 2017-08-14T14:00:52Z

src/cli/commands/init.js

@@ -141,6 +152,9 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
    }

    if (answer) {
+      if (manifestKey === 'private') {
+        answer = yn(answer);


We probably should make use of yn more since we added it?

@rally25rs

I haven't seen @rally25rs' review. He has good points so resigning.

pierreneter · 2017-08-14T18:08:29Z

I think we need a specific document to describe my idea of implementation. And a document on the site to guide users to use this flag.
The cases in my idea that I want to clarify.

yarn init -y

{
  "name": "test-yarn",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT"
}

yarn init -yp

{
  "name": "test-yarn",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "private": true
}

yarn init -p
yarn init v1.0.0
question name (test-yarn):
question version (1.0.0):
question description:
question entry point (index.js):
question repository url:
question author:
question license (MIT):
success Saved package.json
Done in 3.85s.

{
  "name": "test-yarn",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "private": true
}

yarn init
yarn init v1.0.0
question name (test-yarn):
question version (1.0.0):
question description:
question entry point (index.js):
question repository url:
question author:
question license (MIT):
question private:
success Saved package.json
Done in 2.30s.

{
  "name": "test-yarn",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT"
}

yarn init
yarn init v1.0.0
question name (test-yarn):
question version (1.0.0):
question description:
question entry point (index.js):
question repository url:
question author:
question license (MIT):
question private: false
success Saved package.json
Done in 5.34s.

{
  "name": "test-yarn",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "private": false
}

yarn init
yarn init v1.0.0
question name (test-yarn):
question version (1.0.0):
question description:
question entry point (index.js):
question repository url:
question author:
question license (MIT):
question private: true
success Saved package.json
Done in 5.34s.

{
  "name": "test-yarn",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "private": true
}

Just need this line in package.json when user needs it.

BYK · 2017-08-23T10:38:38Z

@rally25rs @pierreneter ping! Do you need anything from us to move this forward? I'd love to get this merged.

rally25rs · 2017-08-23T11:27:52Z

@BYK I would go refactor out some of the conditionals myself, but I don't think there is a way for me to directly modify this PR?

pierreneter · 2017-08-23T17:56:59Z

Hi,
@rally25rs you can create a PR to my fork, here: https://github.com/pierreneter-repositories/yarn/tree/init/private
I will merge it, no matter how.

pierreneter · 2017-08-24T07:40:04Z

@rally25rs
So, I move yn to array of question like you want. And consolidate the tests to make sure all I want.
You can do anything free to satisfy the above cases that I give: #4165 (comment) or agree with what I do.
Please respond soon.
This RP has taken 10 days.
When I respond, no one responds. You can not ask others to do things and ignore them, not listen to them.

rally25rs

Sorry for the delay on this @pierreneter , I didn't realize you were waiting on me to reply. Looks good, and we appreciate your contribution!

rally25rs · 2017-08-24T12:37:10Z

The failing CircleCI tests seems unrelated to this PR, and passes on other integrations.

@BYK I can't merge this unless those pass. I also can't trigger a rebuild on CircleCI because I don't have permissions. Could you re-kick Circle? (or just merge this is github gives you that ability)

BYK · 2017-08-24T13:44:17Z

or just merge this is github gives you that ability

Never, unless I really have to :D

BYK · 2017-08-24T13:46:53Z

When I respond, no one responds. You can not ask others to do things and ignore them, not listen to them.

Hey @pierreneter, sorry for the communication issues. It is easy to miss notifications from GitHub as they tend to be a lot, for active projects like Yarn. Would you like to join our Discord server so you can ping members directly in case they don't notice the updates? Also, apologies, since I try to go over all open PRs regularly to make sure they are not forgotten and I missed this one.

Thanks a lot for your hard work and sticking to the end with this one!

**Summary** Closes yarnpkg#4113. This PR create the `private` flag for `yarn init` command. Can use both `yes` and `private` flags. Or can use `private` flag alone to set `private` is `true` when run command. **Test plan** Have added tests

pierreneter added 2 commits August 14, 2017 14:22

add private flag for yarn init command

be1eda4

add tests for private flag, git init command

666c8b7

rally25rs reviewed Aug 14, 2017

View reviewed changes

rally25rs requested changes Aug 14, 2017

View reviewed changes

BYK previously approved these changes Aug 14, 2017

View reviewed changes

kaylie-alexa mentioned this pull request Aug 23, 2017

yarn init --private #4113

Closed

Add private flag update code

5e4b024

Fix lint

70c82b8

rally25rs approved these changes Aug 24, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into init/private

1d385cb

BYK merged commit 6bab5cc into yarnpkg:master Aug 24, 2017

pierreneter mentioned this pull request Sep 9, 2017

Update introduce for private flag of git init command yarnpkg/website#643

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add private flag for yarn init command #4165

Add private flag for yarn init command #4165

pierreneter commented Aug 14, 2017

rally25rs Aug 14, 2017

pierreneter Aug 14, 2017

rally25rs Aug 23, 2017

rally25rs Aug 14, 2017 •

edited

Loading

pierreneter Aug 14, 2017

rally25rs Aug 14, 2017

pierreneter Aug 14, 2017

rally25rs Aug 14, 2017

pierreneter Aug 24, 2017

rally25rs Aug 24, 2017

rally25rs Aug 14, 2017

pierreneter Aug 14, 2017

rally25rs Aug 14, 2017

pierreneter Aug 24, 2017

rally25rs Aug 14, 2017 •

edited

Loading

rally25rs left a comment •

edited

Loading

BYK left a comment

BYK Aug 14, 2017

pierreneter commented Aug 14, 2017

BYK commented Aug 23, 2017

rally25rs commented Aug 23, 2017

pierreneter commented Aug 23, 2017

pierreneter commented Aug 24, 2017

rally25rs left a comment

rally25rs commented Aug 24, 2017

BYK commented Aug 24, 2017

BYK commented Aug 24, 2017

Add private flag for yarn init command #4165

Add private flag for yarn init command #4165

Conversation

pierreneter commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rally25rs Aug 14, 2017 • edited Loading

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

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

rally25rs Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

rally25rs left a comment • edited Loading

Choose a reason for hiding this comment

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreneter commented Aug 14, 2017

BYK commented Aug 23, 2017

rally25rs commented Aug 23, 2017

pierreneter commented Aug 23, 2017

pierreneter commented Aug 24, 2017

rally25rs left a comment

Choose a reason for hiding this comment

rally25rs commented Aug 24, 2017

BYK commented Aug 24, 2017

BYK commented Aug 24, 2017

rally25rs Aug 14, 2017 •

edited

Loading

rally25rs Aug 14, 2017 •

edited

Loading

rally25rs left a comment •

edited

Loading