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

[FEATURE] Add "ui5 use/add" commands #315

Merged
merged 37 commits into from
Apr 1, 2020
Merged

[FEATURE] Add "ui5 use/add" commands #315

merged 37 commits into from
Apr 1, 2020

Conversation

matz3
Copy link
Member

@matz3 matz3 commented Mar 26, 2020

No description provided.

@matz3
Copy link
Member Author

matz3 commented Mar 26, 2020

Depends on SAP/ui5-project#290

lib/cli/commands/use.js Show resolved Hide resolved
lib/framework/add.js Outdated Show resolved Hide resolved
lib/framework/add.js Outdated Show resolved Hide resolved
lib/framework/use.js Show resolved Hide resolved
lib/framework/use.js Show resolved Hide resolved
@matz3 matz3 marked this pull request as ready for review March 31, 2020 12:44
@coveralls
Copy link

coveralls commented Mar 31, 2020

Coverage Status

Coverage increased (+4.9%) to 92.876% when pulling e641f26 on add-command-use into f18f07c on master.

lib/cli/commands/add.js Outdated Show resolved Hide resolved
lib/cli/commands/add.js Outdated Show resolved Hide resolved
type: "string"
}).option("development", {
describe: "Add as development dependency",
alias: "D",
Copy link
Member

Choose a reason for hiding this comment

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

Why uppercase? For the other commands, the shortcut has the same case as the option, as far as I can see.

I find consistency within our cli more appealing than consistency of a single command with yarn. Or can we have two aliases?

Choose a reason for hiding this comment

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

Will this feature make it into the 2.0 release?

Choose a reason for hiding this comment

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

@codeworrior - it is not only yarn - moreover its also consistency with npm:
https://docs.npmjs.com/cli/install

So, i personally like the abbreviation with the uppercase alias. 👍

Copy link
Member

@RandomByte RandomByte Mar 31, 2020

Choose a reason for hiding this comment

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

Yes, we looked at the aliases of the npm install command here. However, I would personally prefer a --dev and --opt alias. We can add as many alias as we like 😏


Will this feature make it into the 2.0 release?

Yes, definitely. Our documentation already mentions it: https://github.com/SAP/ui5-tooling/pull/221/files#diff-1a523bd9fa0dbf998008b37579210e12R32

Choose a reason for hiding this comment

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

Great, thanks! So we just add all of them.

Copy link
Member

Choose a reason for hiding this comment

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

I'll discuss our alias options with @matz3 in the morning

Copy link

@petermuessig petermuessig Apr 1, 2020

Choose a reason for hiding this comment

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

Looking into yarn, they call --opt => --optional -> I would try to align myself as much as possible with already existing CLIs. This helps people to easily use the one or the other. The uppercase aliases are common across npm and yarn and that is why I am also in favor to have them.

@matz3 matz3 changed the title [FEATURE] Add ui5 use command [FEATURE] Add "ui5 use/add" commands Apr 1, 2020
lib/cli/commands/add.js Outdated Show resolved Hide resolved
lib/cli/commands/add.js Outdated Show resolved Hide resolved
lib/cli/commands/use.js Outdated Show resolved Hide resolved
@matz3 matz3 merged commit 920fbfc into master Apr 1, 2020
@matz3 matz3 deleted the add-command-use branch April 1, 2020 09:41
tobiasso85 added a commit that referenced this pull request Apr 16, 2020
Followup of #315.
Add ui5 remove command which removes libraries from
the framework libraries section in the ui5.yaml.
tobiasso85 added a commit that referenced this pull request Apr 22, 2020
* [FEATURE] Add "ui5 remove" command

Followup of #315.
Add ui5 remove command which removes libraries from
the framework libraries section in the ui5.yaml.
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.

5 participants