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

Add private flag for yarn init command #4165

Merged
merged 5 commits into from
Aug 24, 2017
Merged

Add private flag for yarn init command #4165

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
be1eda4
add private flag for yarn init command
pierreneter Aug 14, 2017
666c8b7
add tests for private flag, git init command
pierreneter Aug 14, 2017
5e4b024
Add private flag update code
pierreneter Aug 24, 2017
70c82b8
Fix lint
pierreneter Aug 24, 2017
1d385cb
Merge remote-tracking branch 'origin/master' into init/private
BYK Aug 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions __tests__/commands/__snapshots__/init.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,21 @@ Object {
"license": "MIT",
"main": "index.js",
"name": "hi-github",
"private": false,
"repository": "https://github.com/user/repo",
"version": "1.0.0",
}
`;

exports[`init-private-empty 1`] = `
Object {
"license": "MIT",
"main": "index.js",
"name": "private-empty",
"version": "1.0.0",
}
`;

exports[`init-yes 1`] = `
Object {
"license": "MIT",
Expand All @@ -18,3 +28,13 @@ Object {
"version": "1.0.0",
}
`;

exports[`init-yes-private 1`] = `
Object {
"license": "MIT",
"main": "index.js",
"name": "init-yes-private",
"private": true,
"version": "1.0.0",
}
`;
66 changes: 66 additions & 0 deletions __tests__/commands/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,36 @@ test.concurrent('init --yes should create package.json with defaults', (): Promi
// Name is derived from directory name which is dynamic so check
// that separately and then remove from snapshot
expect(manifest.name).toEqual(path.basename(cwd));
expect(manifest.private).toEqual(undefined);
expect({...manifest, name: 'init-yes'}).toMatchSnapshot('init-yes');
},
);
});

test.concurrent('init --yes --private should create package.json with defaults and private true', (): Promise<void> => {
return buildRun(
ConsoleReporter,
fixturesLoc,
(args, flags, config, reporter, lockfile): Promise<void> => {
return runInit(config, reporter, flags, args);
},
[],
{yes: true, private: true},
'init-yes-private',
async (config): Promise<void> => {
const {cwd} = config;
const manifestFile = await fs.readFile(path.join(cwd, 'package.json'));
const manifest = JSON.parse(manifestFile);

// Name is derived from directory name which is dynamic so check
// that separately and then remove from snapshot
expect(manifest.name).toEqual(path.basename(cwd));
expect(manifest.private).toEqual(true);
expect({...manifest, name: 'init-yes-private'}).toMatchSnapshot('init-yes-private');
},
);
});

test.concurrent('init using Github shorthand should resolve to full repository URL', (): Promise<void> => {
const questionMap = Object.freeze({
name: 'hi-github',
Expand All @@ -44,6 +69,7 @@ test.concurrent('init using Github shorthand should resolve to full repository U
'repository url': 'user/repo',
author: '',
license: '',
private: 'false',
});
class TestReporter extends ConsoleReporter {
question(question: string, options?: QuestionOptions = {}): Promise<string> {
Expand Down Expand Up @@ -75,6 +101,46 @@ test.concurrent('init using Github shorthand should resolve to full repository U
);
});

test.concurrent('init and give private empty', (): Promise<void> => {
const questionMap = Object.freeze({
name: 'private-empty',
version: '',
description: '',
'entry point': '',
'repository url': '',
author: '',
license: '',
private: '',
});
class TestReporter extends ConsoleReporter {
question(question: string, options?: QuestionOptions = {}): Promise<string> {
return new Promise((resolve, reject) => {
const parsedQuestion = question.replace(/ \((.*?)\)/g, '');
if (parsedQuestion in questionMap) {
resolve(questionMap[parsedQuestion]);
} else {
reject(new Error(`Question not found in question-answer map ${parsedQuestion}`));
}
});
}
}

return buildRun(
TestReporter,
fixturesLoc,
(args, flags, config, reporter, lockfile): Promise<void> => {
return runInit(config, reporter, flags, args);
},
[],
{},
'init-github',
async (config): Promise<void> => {
const manifestFile = await fs.readFile(path.join(config.cwd, 'package.json'));
expect(JSON.parse(manifestFile)).toMatchSnapshot('init-private-empty');
},
);
});

test.concurrent('getGitConfigInfo should not return the git config val', async (): Promise<void> => {
expect('hi seb').toEqual(await getGitConfigInfo('some-info', () => Promise.resolve('hi seb')));
});
Expand Down
1 change: 1 addition & 0 deletions __tests__/fixtures/init/init-yes-private/.npmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
yarn-offline-mirror=./mirror-for-offline
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@
"tar-stream": "^1.5.2",
"uuid": "^3.0.1",
"v8-compile-cache": "^1.1.0",
"validate-npm-package-license": "^3.0.1"
"validate-npm-package-license": "^3.0.1",
"yn": "^2.0.0"
},
"devDependencies": {
"babel-core": "^6.24.1",
Expand Down
19 changes: 17 additions & 2 deletions src/cli/commands/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ import * as validate from '../../util/normalize-manifest/validate.js';

const objectPath = require('object-path');
const path = require('path');
const yn = require('yn');

export function setFlags(commander: Object) {
commander.option('-y, --yes', 'use default options');
commander.option('-p, --private', 'use default options and private true');
}

export function hasWrapper(commander: Object, args: Array<string>): boolean {
Expand Down Expand Up @@ -89,12 +91,18 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
question: 'license',
default: String(config.getOption('init-license')),
},
{
key: 'private',
question: 'private',
default: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

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").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@rally25rs rally25rs Aug 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ''.

inputFormatter: yn,
},
];

// get answers
const pkg = {};
for (const entry of keys) {
const {yes} = flags;
const {yes, private: privateFlag} = flags;
const {key: manifestKey} = entry;
let {question, default: def} = entry;

Expand All @@ -114,8 +122,12 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
def = val;
}

if (manifestKey === 'private' && privateFlag) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

def = true;
}

if (def) {
question += ` (${def})`;
question += ` (${def.toString()})`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the additional .toString()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

let answer;
Expand All @@ -141,6 +153,9 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
}

if (answer) {
if (entry.inputFormatter) {
answer = entry.inputFormatter(answer);
}
objectPath.set(pkg, manifestKey, answer);
}
}
Expand Down
4 changes: 4 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4958,3 +4958,7 @@ yargs@~3.10.0:
cliui "^2.1.0"
decamelize "^1.0.0"
window-size "0.1.0"

yn@^2.0.0:
version "2.0.0"
resolved "https://registry.yarnpkg.com/yn/-/yn-2.0.0.tgz#e5adabc8acf408f6385fc76495684c88e6af689a"