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

Breaking: rename every yarn <command> rm to yarn <command> remove #3989

Merged
merged 3 commits into from
Jul 25, 2017
Merged

Breaking: rename every yarn <command> rm to yarn <command> remove #3989

merged 3 commits into from
Jul 25, 2017

Conversation

tpina
Copy link
Contributor

@tpina tpina commented Jul 22, 2017

Summary

From #3952 - For the sake of consistency and clarity, we need to rename the rm command to remove and add a deprecation warning for rm.

Test plan
I have done some rounds of functional manual testing.

Unfortunately, every command with rm requires a login. Coincidentally, and if I am not mistaken, none of the commands that implement rm has tests ( owner, tag, team).

I will happily add tests for those but I can't seem to make a mock login to work... I need suggestions/guidance?

Note: I'm working on a windows machine. When I try to run yarn run test I get the following

$ yarn lint && yarn test-only
Found 0 errors
C:\code\projects\yarn\node_modules\.bin\jest:2
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")
          ^^^^^^^
SyntaxError: missing ) after argument list
    at Object.exports.runInThisContext (vm.js:76:16)
    at Module._compile (module.js:542:28)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:394:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:509:3
error Command failed with exit code 1.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@tpina
Copy link
Contributor Author

tpina commented Jul 22, 2017

@BYK and @voxsim

Waiting for feedback

@BYK
Copy link
Member

BYK commented Jul 23, 2017

Note: I'm working on a windows machine. When I try to run yarn run test I get the following

Looks like your jest installation is a bit weird. Try removing that node_modules/.bin/jest file and replace it with the jest.cmd which should already be there. If not we'll figure it out.

@BYK BYK requested a review from voxsim July 23, 2017 13:51
@BYK BYK self-assigned this Jul 23, 2017
Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

LGTM except for the export parts.

@voxsim, I'll let you do the honors :)

return found;
},
);
remove(config: Config, reporter: Reporter, flags: Object, args: Array<string>): Promise<boolean> {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just export remove directly?

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 wanted to keep consistency with the ls changes.

I have noticed that I forgot to change

['add <user> [[<@scope>/]<pkg>]', 'rm <user> [[<@scope>/]<pkg>]', 'ls [<@scope>/]<pkg>']

Should I do it? Also, ls is still there, should I change it as part of another issue/PR?

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 defer any ls changes to their own PR. Also, okay for now for consistency but I think we can do better.

May be you'd be interested in cleaning that up in a follow-up PR?

@BYK
Copy link
Member

BYK commented Jul 23, 2017

Thanks a lot!

@tpina
Copy link
Contributor Author

tpina commented Jul 23, 2017

@BYK and @voxsim did a small update so that command templates are in line with the new specification. How can I update the website as well?

@BYK
Copy link
Member

BYK commented Jul 25, 2017

@tpina - The website is maintained in a different repository: https://github.com/yarnpkg/website/

Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Still LGTM, now including the export parts since I understand it was done for consistency.

@voxsim I'll merge this EOD today if you don't chime in and we can address any objections in follow-ups.

return found;
},
);
remove(config: Config, reporter: Reporter, flags: Object, args: Array<string>): Promise<boolean> {
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 defer any ls changes to their own PR. Also, okay for now for consistency but I think we can do better.

May be you'd be interested in cleaning that up in a follow-up PR?

@voxsim
Copy link
Contributor

voxsim commented Jul 25, 2017

@BYK I don't have any objections, sorry but I am busy these days, @tpina thanks for the pr!

@tpina
Copy link
Contributor Author

tpina commented Jul 25, 2017

I'd defer any ls changes to their own PR. Also, okay for now for consistency but I think we can do better.
May be you'd be interested in cleaning that up in a follow-up PR?

@BYK Yes, not a problem. What do you mean that "we can do better" - Should I just export remove? Let me know what you want done. I have some time this week to do it.

@BYK
Copy link
Member

BYK commented Jul 25, 2017

@BYK I don't have any objections, sorry but I am busy these days, @tpina thanks for the pr!

No worries! Just making sure you're still in the loop :)

@BYK BYK merged commit d56c6db into yarnpkg:master Jul 25, 2017
@tpina tpina deleted the 3952-rm-fix branch July 25, 2017 19:05
@BYK BYK mentioned this pull request Sep 27, 2017
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