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

More hub-consistent merge-request method names #40

Conversation

stevelacey
Copy link
Contributor

I think these are better, obviously this is a breaking change and you're welcome to ignore me, but these are consistent with hub

@stevelacey stevelacey mentioned this pull request Nov 24, 2017
@stevelacey stevelacey force-pushed the rename-merge-request-commands branch 3 times, most recently from 5b49330 to 8e6b631 Compare November 24, 2017 13:02
@stevelacey
Copy link
Contributor Author

You could alias the old names or vice versa (theres a method for that) but it makes the help output a bit verbose and given this is a cli tool I'm not sure a BC break like this is a massive deal

@vishwanatharondekar
Copy link
Owner

@stevelacey I see that commandr has support for alias. https://github.com/tj/commander.js/blob/master/examples/deploy#L27

I think it will be good idea to use that rather than doing these changes.

@stevelacey
Copy link
Contributor Author

stevelacey commented Nov 25, 2017

@vishwanatharondekar I can add the aliases in but alias is normally for adding terser options rather than alternatives – and it makes the help output pretty beefy with the command output listing:

merge-request|create-merge-request   blah blah blah
merge-request|open-merge-requests    blah blah blah

Also note you can only have one alias per command currently in commander. I would argue for merge-request a better alias would be mr and you'd loose that opportunity with this change

@vishwanatharondekar
Copy link
Owner

What you are saying makes sense. As per my information I have some users using this in CI with print option. It will break for them since they are installing from github url. If we had npm and versions earlier we could have changed the version to 2.x and that could have helped maybe.

I will keep this open for a while to come at conclusion.

@stevelacey stevelacey force-pushed the rename-merge-request-commands branch 4 times, most recently from 37f03d8 to 0412861 Compare November 25, 2017 15:06
@stevelacey
Copy link
Contributor Author

stevelacey commented Nov 25, 2017

@vishwanatharondekar how about with this legacy aliases patch? Allows them to continue working – but doesn't advertise them in the help output 🙆‍♂️

Also adds mr and mrs aliases

@vishwanatharondekar
Copy link
Owner

vishwanatharondekar commented Nov 25, 2017

This is so smart and bit ugly (global legacies) that I like it 😄 . Give me some time to test a bit and I will merge. It must be awesome working with you.

@vishwanatharondekar
Copy link
Owner

vishwanatharondekar commented Nov 25, 2017

I am merging this. By the way wouldn't it make more sense to add .alias in commandr as well.

@vishwanatharondekar vishwanatharondekar merged commit f2584de into vishwanatharondekar:master Nov 25, 2017
@stevelacey
Copy link
Contributor Author

@vishwanatharondekar what do you mean add .alias to commandr?

@vishwanatharondekar
Copy link
Owner

vishwanatharondekar commented Nov 25, 2017

I meant a pull request in https://github.com/tj/commander.js

@stevelacey
Copy link
Contributor Author

stevelacey commented Nov 25, 2017 via email

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.

None yet

2 participants