Use "-v" to display version number instead of "-V" #3395

g-plane · 2017-05-13T02:30:40Z

Summary

Before, we run yarn -V or yarn --version to display version number of Yarn. But we are used to using -v to display it, like running node -v and npm -v.
Maybe caused by commander.js, flags of command for displaying version number cannot be '-v, -V, --version'. In other word, we cannot run yarn -V to display version number any more.

Test plan

Just run yarn -v. Note that do NOT use yarn -V any more.

$ yarn -v
0.25.0-0
$ yarn --version
0.25.0-0

bestander · 2017-05-13T20:38:18Z

Any idea how to make it non breaking?
Would be great to support -V

g-plane · 2017-05-14T00:52:53Z

Now is OK.

arcanis · 2017-05-15T10:27:46Z

I don't really agree; -v is either --version or --verbose, depending on the tool you use. Since the version can already be extracted via -V and/or --version, I would prefer for -v to stay as the verbose short option. It would also allow us to use the -vv / -vvv syntax to control the level of verbosity.

g-plane · 2017-05-15T11:54:31Z

But it seems that at current version running with -v, it will run install actually, not --verbose. For most people, after installed Yarn or other tools with CLI, they may run -v to test if the tool has been installed successfully, not just to get the version number.

bestander · 2017-05-15T11:59:04Z

Ok, considering that opinions are split regarding -v/-V I propose to get rid of both and use only long version for arguments.

I agree that it is annoying when running yarn -v it executes yarn install, even worse it saves an empty lockfile.
Let's discuss fixing that effect then?

g-plane · 2017-05-15T12:08:19Z

Do you mean that do not execute install if current directory doesn't have lockfile?

bestander · 2017-05-15T12:09:23Z

Maybe if there is not package.json.
Missing yarn.lock can happen

arcanis · 2017-05-15T12:11:55Z

Or we could just make yarn print Yarn's usage (ie require the command name instead of fallbacking on install). By doing this, we could actually support both -v as --version and --verbose:

$> yarn
Usage: yarn [command] [... options]
Commands: ...

$> yarn -v
0.24.3

$> yarn -V
0.24.3

$> yarn install -v
[info] Fetching http://...

$> yarn install -vv
[info] Fetching http://...
[debug] Receiving HTTP 200 from ...

bestander · 2017-05-15T12:13:28Z

I quite like that install is the default command of yarn, I use it a lot and it would be a noticeable breaking change IMHO

g-plane · 2017-05-15T12:15:49Z

I like install as a default command too.

g-plane · 2017-05-15T12:36:43Z

@arcanis I think the example you showed is a good idea, except the first usage.

bestander · 2017-05-23T17:41:05Z

Any thoughts on how to proceed?

g-plane · 2017-05-24T00:02:49Z

Do you prefer to making Yarn not execute install if missing package.json? If so, I will try doing.

bestander · 2017-05-24T10:24:01Z

This is nuanced.
Running yarn add in a folder without package.json is fine.
Running yarn install is not.
However we work on workspaces where a project can have multiple package.json files, in that case Yarn should find the top level folder and run install from there.

bestander · 2017-05-24T10:27:35Z

Here we have 2 issues, I's like us to get somewhere about -v/-V.
@arcanis, @g-plane, how about we phase out ambiguous -v/-V completely?
And print usage message, e.g. Use --version to print version and --verbose to install verbously

g-plane · 2017-05-24T10:45:22Z

I think using -v is convenient to check if yarn has been installed successfully, not only display version number. Maybe using -v to check that for any tools is a habit, I think.

bestander · 2017-05-24T10:46:16Z

Well -v could print a version and show help message :)

…

On 24 May 2017 at 11:45, Pig Fang ***@***.***> wrote: I think using -v is convenient to check if yarn has been installed successfully, not only display version number. Maybe using -v to check that for any tools is a habit, I think. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3395 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWGwomKizN5zta4rgncqSUVzBwsnoks5r9ApMgaJpZM4NZ738> .

g-plane · 2017-05-24T10:49:28Z

How about making running yarn -v only it will display version and running yarn install -v it will actually execute yarn install --verbose?

g-plane · 2017-06-01T02:44:23Z

How should I add tests?

bestander · 2017-06-15T19:02:58Z

@g-plane, sorry for a delay, I was in a bug crunch and then a time off.
Are you still available to do more?

I think there is not need to add automated tests.
Showing screenshots of all possible scenarios in the PR description should be enough.
How about making running yarn -v only it will display version and running yarn install -v it will actually execute yarn install --verbose?

Sounds non consistent, people are used to have install command as default, e.g. yarn --force, so -v behaving differently may be more confusing.

On that note, when you do yarn -v it outputs

yarn -v
yarn install v0.24.3
info No lockfile found.
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...

success Saved lockfile.
✨  Done in 0.05s.

Which shows version of Yarn.
The only annoying side effect is that it adds an empty yarn.lock file.
Can we fix just that, e.g. don't save the lock file when there are no packages?

g-plane · 2017-06-16T02:34:01Z

@bestander Well, I have done something at the last commit, which make yarn won't save lock file. Here is the code: ff00684

The first line if (Object.getPrototypeOf(this) === Install.prototype) is to check the current command. If it is install, it won't save lock file. But if the command is add, it will do what yarn add should do and save yarn.lock.

Anything should be changed? I will do.

Screenshots:

bestander · 2017-06-16T22:43:15Z

src/cli/index.js

@@ -29,7 +29,7 @@ const args = process.argv.slice(2, doubleDashIndex === -1 ? process.argv.length
 const endArgs = doubleDashIndex === -1 ? [] : process.argv.slice(doubleDashIndex + 1, process.argv.length);

 // set global options
-commander.version(version);
+commander.version(version, '-v, -V, --version');


let's not alias -v/-V to --version yet because we have not agreed whether people will confuse it with --verbose.

Only use --version ?

Yeah, we avoid ambiguous shorthands in Yarn.

bestander · 2017-06-16T23:18:30Z

src/cli/commands/install.js

+
+        // `fs.exists()` is deprecated since nodejs v8
+        try {
+          await fs.access(loc);


looks a bit radical, I think this may be done coming from another angle.
If package.json does not exist then patterns from fetchRequestFromCwd call would be empty.

Okay, how should I change?

Do you mean I should call the function fetchRequestFromCwd?

No, it is called on line 421.
depRequests contains all the patterns from a package.json.
If it is empty and it is not an Add command then installation should stop without saving a lockfile.

g-plane · 2017-06-17T08:34:10Z

@bestander I have done a force push for the commit: 93ddc5d . You can review the code. If something should be changed, I will do.

Screenshot:

g-plane · 2017-06-17T09:03:48Z

It seems there are some conflicts with unit test.

bestander · 2017-06-21T23:49:20Z

Yeah, a few tests were relying on previous behavior

g-plane · 2017-06-21T23:50:40Z

So should I change the test?

bestander · 2017-06-21T23:51:01Z

src/cli/commands/install.js

@@ -431,6 +431,10 @@ export class Install {
      this.scripts.setArtifacts(artifacts);
    }

+    if (Object.getPrototypeOf(this) === Install.prototype && depRequests.length === 0) {


Look at code on line 367, it saves explicitly when there is nothing to install.
Maybe that logic should be there?

Maybe I should move the code to there?

I think you can just enhance the code there, you probably don't need to check for Install.prototype.
The logic should be "don't save lockfile if there are no dependencies"

Ok. I will try.

bestander · 2017-06-21T23:51:13Z

src/cli/index.js

@@ -29,7 +29,7 @@ const args = process.argv.slice(2, doubleDashIndex === -1 ? process.argv.length
 const endArgs = doubleDashIndex === -1 ? [] : process.argv.slice(doubleDashIndex + 1, process.argv.length);

 // set global options
-commander.version(version);
+commander.version(version, '--version');


what this command changes?

Remove -V
https://github.com/tj/commander.js/blob/master/index.js#L834

bestander · 2017-06-21T23:53:20Z

So should I change the test?

Yes, as long as the change is reasonable.

bestander · 2017-06-23T01:28:44Z

Ok, this looks good now, thanks @g-plane.
Could you rebase please?
And could you confirm what message is written when there is no package.json and user types yarn -v?

g-plane · 2017-06-23T01:32:40Z

How about: No package.json found. ?

bestander · 2017-06-23T01:33:47Z

Maybe not needed. What does it output now?

…

On 22 June 2017 at 18:32, Pig Fang ***@***.***> wrote: How about: No package.json found. ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3395 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWLPcI_mW-6uy4dVrHSxH9QJlohhqks5sGxXDgaJpZM4NZ738> .

g-plane · 2017-06-23T01:35:19Z

bestander · 2017-06-23T01:40:00Z

Yeah, then no package.json found sounds good.

g-plane · 2017-06-23T11:00:54Z

Sorry for delay.

bestander · 2017-06-23T23:18:27Z

Can you run yarn prettier to make code comply with the styleguide?

bestander · 2017-06-23T23:20:50Z

src/cli/commands/install.js

-    await this.saveLockfileAndIntegrity(topLevelPatterns, workspaceLayout);
+    if (topLevelPatterns.length || await fs.exists(path.join(this.config.cwd, constants.LOCKFILE_FILENAME))) {
+      await this.saveLockfileAndIntegrity(topLevelPatterns, workspaceLayout);
+    }


how about

} else { reporter.info(reporter.lang('noPackageJsonFound')); }

Then the the block at line 843-846 can be removed

I think I missed this one, do you need to check await fs.exists(path.join(this.config.cwd, constants.LOCKFILE_FILENAME)?

Check what? I am sorry that I do not quite understand what you mean.

Why are you doing this

if (topLevelPatterns.length || await fs.exists(path.join(this.config.cwd, constants.LOCKFILE_FILENAME))) {

instead of this?

if (topLevelPatterns.length) { }

What does the second condition mean?

When running remove command, yarn will update lock file by running install.
https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/remove.js#L74

I see, good thinking.
Yes, makes sense in that case.
Would you add a comment for that?

Ok, I will.

One more thing, fs.exists() is deprecated since node 8. Should I make a change?
https://nodejs.org/dist/latest-v8.x/docs/api/fs.html#fs_fs_exists_path_callback

no, we use our own wrapper of fs.exists, so it is fine

bestander · 2017-06-23T23:23:16Z

src/reporters/lang/en.js

@@ -181,6 +181,8 @@ const messages = {
  foundWarnings: 'Found $0 warnings.',
  foundErrors: 'Found $0 errors.',

+  noPackageJsonFound: 'No package.json found.',


How about

notSavedLockfileNoDependencies: 'Lockfile not saved, no dependencies.',

We are not focusing on a package.json existence but on the packages that were installed

bestander · 2017-06-23T23:24:17Z

Good job, @g-plane, just a few nits.

g-plane · 2017-06-24T13:44:25Z

I have run yarn prettier and yarn lint before committing, but the tests still failed. I don't know why.

bestander · 2017-06-26T18:35:07Z

Try rebasing to master, yarn install && yarn prettier and see if there are any file changes

g-plane · 2017-06-27T10:54:47Z

Is that OK?

bestander · 2017-07-01T03:25:07Z

Great job, @g-plane!
Looking forward for more PRs.

g-plane · 2017-07-01T03:25:47Z

Thanks!

g-plane · 2017-07-01T03:44:29Z

Should #2710 and #3329 be closed?

Do not save lockfile when no dependencies (yarnpkg#3395)

…encies this reverts most of yarnpkg#3395 992b5c9 which appears only necessary to work around yarnpkg#3329 (requesting the yarn version with -v triggered install)

…5843) **Summary** this reverts most of #3395 992b5c9 which appears only necessary to work around #3329 (requesting the yarn version with -v triggered install). As noted in #5839, `yarn -v` and `yarn check` no longer have `install` as a side effect and IMO, the original behavior where an empty lock file is generated as part of install is the correct behavior. Many tools currently sniff for a yarn.lock file to determine if yarn is being used. It seems more consistent with `yarn import` which generates a lockfile even if the existing node_modules is empty. fixes #5839 **Test plan** `yarn run test` passes `yarn run lint` passes `yarn-local install` saves an empty lockfile `yarn-local import` continues to save an empty lockfile `yarn-local -v` will not trigger install and will not generate a lock file. correctly displays a version. `yarn-local check` will not trigger install and will not generate a lock file

bestander reviewed Jun 16, 2017

View reviewed changes

bestander reviewed Jun 21, 2017

View reviewed changes

bestander reviewed Jun 23, 2017

View reviewed changes

Do not save lockfile when no dependencies

992b5c9

bestander merged commit e0e119e into yarnpkg:master Jul 1, 2017

GulajavaMinistudio added a commit to GulajavaMinistudio/yarn that referenced this pull request Jul 1, 2017

Merge pull request #67 from yarnpkg/master

44a6969

Do not save lockfile when no dependencies (yarnpkg#3395)

bestander mentioned this pull request Jul 5, 2017

Return yarn version with yarn -v #3818

Closed

jimmyhchan mentioned this pull request May 21, 2018

feat(cli): Generate an empty lockfile when there are no dependencies #5843

Merged

Use "-v" to display version number instead of "-V" #3395

Use "-v" to display version number instead of "-V" #3395

Conversation

g-plane commented May 13, 2017

bestander commented May 13, 2017

g-plane commented May 14, 2017 • edited Loading

arcanis commented May 15, 2017 • edited Loading

g-plane commented May 15, 2017

bestander commented May 15, 2017

g-plane commented May 15, 2017

bestander commented May 15, 2017

arcanis commented May 15, 2017 • edited Loading

bestander commented May 15, 2017

g-plane commented May 15, 2017

g-plane commented May 15, 2017

bestander commented May 23, 2017

g-plane commented May 24, 2017

bestander commented May 24, 2017

bestander commented May 24, 2017

g-plane commented May 24, 2017

bestander commented May 24, 2017 via email

g-plane commented May 24, 2017

g-plane commented Jun 1, 2017

bestander commented Jun 15, 2017 • edited Loading

g-plane commented Jun 16, 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

g-plane commented Jun 17, 2017 • edited Loading

g-plane commented Jun 17, 2017

bestander commented Jun 21, 2017

g-plane commented Jun 21, 2017

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

bestander commented Jun 21, 2017

bestander commented Jun 23, 2017

g-plane commented Jun 23, 2017

bestander commented Jun 23, 2017 via email

g-plane commented Jun 23, 2017

bestander commented Jun 23, 2017

g-plane commented Jun 23, 2017

bestander commented Jun 23, 2017

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

bestander commented Jun 23, 2017

g-plane commented Jun 24, 2017

bestander commented Jun 26, 2017

g-plane commented Jun 27, 2017

bestander commented Jul 1, 2017

g-plane commented Jul 1, 2017

g-plane commented Jul 1, 2017

g-plane commented May 14, 2017 •

edited

Loading

arcanis commented May 15, 2017 •

edited

Loading

arcanis commented May 15, 2017 •

edited

Loading

bestander commented Jun 15, 2017 •

edited

Loading

g-plane commented Jun 16, 2017 •

edited

Loading

g-plane commented Jun 17, 2017 •

edited

Loading