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.

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

yarn <command> ls command cleanup #4045

Merged
merged 4 commits into from
Jul 31, 2017
Merged

yarn <command> ls command cleanup #4045

merged 4 commits into from
Jul 31, 2017

Conversation

tpina
Copy link
Contributor

@tpina tpina commented Jul 28, 2017

Summary

From #3898 (comment)

This PR is to clean up the remaining undeprecated yarn <command> ls command and update the usage template.

Test plan

Some rounds of functional manual testing to assert that the correct messages are displayed and previous commands work as per expected. Touched files do not have any associated tests.

@tpina
Copy link
Contributor Author

tpina commented Jul 28, 2017

@BYK and @voxsim

Done. Awaiting for feedback.

function warnDeprecation(reporter: Reporter) {
reporter.warn(`\`yarn team rm\` is deprecated. Please use \`yarn team remove\`.`);
function warnDeprecation(reporter: Reporter, deprecationWarning: DeprecationWarning) {
reporter.warn(`\`yarn team ${deprecationWarning.deprecatedCommand}\` is deprecated.
Copy link
Member

Choose a reason for hiding this comment

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

You should be using reporter.lang and add a language key for these so they can be translated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok not a problem!

function wrapRequired(
callback: CLIFunctionWithParts,
requireTeam: boolean,
subCommandDeprecated?: DeprecationWarning,
Copy link
Member

Choose a reason for hiding this comment

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

I'd rename this to deprecationMessage or deprecationInfo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

flags: Object,
args: Array<string>,
): CLIFunctionReturn {
if (args.length === 1) {
Copy link
Member

Choose a reason for hiding this comment

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

return args.length === 1 && callback(...);

Also would be nice to explain why we do this only when argument count is exactly 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yarn team list only takes one arg which is either: <scope> or <scope:team> more than one arg and the command should fail.

@BYK
Copy link
Member

BYK commented Jul 28, 2017

@tpina thanks a lot! This looks good to me. Would you like to address my changes or just merge it as it is?

@tpina
Copy link
Contributor Author

tpina commented Jul 28, 2017

@BYK I'll address your changes/comments. Let me take another look at it.

@tpina
Copy link
Contributor Author

tpina commented Jul 28, 2017

@BYK a really annoying thing with the reporter.lang - it keeps putting quote on the params so the final string reads: warning yarn "team" "ls" is deprecated. Please use yarn "team" "list".
I was trying to do something general without dedicated messages...

Is there any way around this without messing around with the regex in the base reporter?

Worst case scenario I'll use dedicated messages.

@tpina
Copy link
Contributor Author

tpina commented Jul 28, 2017

meh... workaround added. See if you agree once checks have completed.

@arcanis arcanis requested a review from voxsim July 28, 2017 17:44
@@ -324,6 +324,8 @@ const messages = {
'Installing Yarn via Yarn will result in you having two separate versions of Yarn installed at the same time, which is not recommended. To update Yarn please follow https://yarnpkg.com/en/docs/install .',

scopeNotValid: 'The specified scope is not valid.',

yarnDeprecatedCommand: '`yarn $0 $1` is deprecated.\n Please use `yarn $0 $2`.',
Copy link
Member

Choose a reason for hiding this comment

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

This would repeat the yarn part in the message :(

I'll fix it and then merge.

@tpina
Copy link
Contributor Author

tpina commented Jul 31, 2017

@BYK you're right. Apologies for that.

@BYK BYK merged commit 9195932 into yarnpkg:master Jul 31, 2017
@tpina tpina deleted the ls-cleanup branch July 31, 2017 15:14
@Lucas-C
Copy link

Lucas-C commented Sep 21, 2017

I think the current doc does not reflect the ls -> list change:
https://yarnpkg.com/en/docs/cli/owner

@BYK
Copy link
Member

BYK commented Sep 26, 2017

@Lucas-C thanks for the heads up! Filed yarnpkg/website#660

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

Successfully merging this pull request may close these issues.

3 participants