Skip to content
This repository has been archived by the owner on Aug 5, 2024. It is now read-only.

Added pattern support tonumbers:list #140

Merged
merged 11 commits into from
Jun 13, 2017
Merged

Added pattern support tonumbers:list #140

merged 11 commits into from
Jun 13, 2017

Conversation

AlexLakatos
Copy link
Contributor

No description provided.

@AlexLakatos AlexLakatos requested a review from leggetter June 13, 2017 12:07
@cbetta
Copy link
Contributor

cbetta commented Jun 13, 2017

What has this been replaced with?

@leggetter
Copy link
Contributor

@cbetta - Thanks for the comment. I'd completely missed that in doing this we lose the numbers:list command which is a breaking change.

@cbetta
Copy link
Contributor

cbetta commented Jun 13, 2017

Probably a good idea to list what something fixes when it's marked as a FIX ;)

@cbetta
Copy link
Contributor

cbetta commented Jun 13, 2017

Also what's up with the commit history here? It's removing one feature with 6 commits, and yet the README not the CHANGELOG is updated.

src/bin.js Outdated
.command('number:list', null, { noHelp: true })
.alias('numbers')
.alias('numbers:list')
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work. You can't double alias (at least you couldn't last year)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

number:buy seems to do the same thing and has 2 aliases.

Copy link
Contributor

@cbetta cbetta Jun 13, 2017

Choose a reason for hiding this comment

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

@AlexLakatos that might be a mistake. Can you test this please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and testing it out on the CLI, it seems the last alias is the only one considered, and all others get to undefined, prompting help to be listed.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems to confirm what I remembered of it. We should probably fix the number:buy one

@cbetta
Copy link
Contributor

cbetta commented Jun 13, 2017

@AlexLakatos so the reason so much code seems duplicate in bin.js is because of this issue:

#14

If you found a solution to this we should probably deduplicate most secondary aliases. I added a lot of aliases to help remove the need to remember if every command is singular (number) or plural (numbers).

@cbetta
Copy link
Contributor

cbetta commented Jun 13, 2017

@AlexLakatos ok to close this issue and open a ticket for the number:buy problem?

@AlexLakatos
Copy link
Contributor Author

@cbetta not really. I'll just change scope to re-add numbers:list and the nl alias to it. Still need to release 0.3.9 Will log a separate issue to check all aliases in the app though.

@cbetta
Copy link
Contributor

cbetta commented Jun 13, 2017

@AlexLakatos can we fix the commit history for this branch? It seems a bit messed up. Probably based on wrong base commit

@AlexLakatos AlexLakatos changed the title Fixed: Removed deprecated numbers:list Added pattern support tonumbers:list Jun 13, 2017
@AlexLakatos
Copy link
Contributor Author

@cbetta wouldn't that be fixed anyway with a squash and merge?

@cbetta
Copy link
Contributor

cbetta commented Jun 13, 2017

@AlexLakatos as long as we do a squash ;)

@AlexLakatos AlexLakatos merged commit 67b97b9 into master Jun 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants