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
Add publisher command #397
Conversation
971427c
to
b462bae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good to me.
Minor review comments inlined. Also make the change to use nap descriptor as discussed in person and I will merge.
docs/cli/cauldron/add/publisher.md
Outdated
#### Syntax | ||
|
||
###### Maven publisher | ||
`ern caundron add publisher --mavenUrl=<url>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo > caundron
docs/cli/cauldron/add/publisher.md
Outdated
`ern caundron add publisher --mavenUrl=<url>` | ||
|
||
###### Github publisher | ||
`ern caundron add publisher --githubUrl=<url>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo > caundron
} else if (githubUrl) { | ||
url = githubUrl | ||
} else { | ||
throw new Error(`Please provide a publisher option (publisher --mavenUrl or --gitHubUrl)`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single quotes ;) ^^
|
||
import inquirer from 'inquirer' | ||
|
||
export async function askUserToChooseAnOption (choices: Array<string>, message?: string): Promise<any> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that. We have quite some calls to inquirer directly, it would be leaner to just reuse this one (and maybe other functions that will be addded to this util) where appropriate (through another "prompts" refactoring PR later on).
Makes it also more simple to replace inquirer lib with another one as every use of it will be centralized (also ease mocking for testing) 👍
docs/cli/cauldron/add/publisher.md
Outdated
@@ -0,0 +1,28 @@ | |||
## `ern cauldron add publihser` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo publisher
docs/cli/cauldron/add/publisher.md
Outdated
|
||
#### Remarks | ||
|
||
* The `ern cauldron add publihser` command is mostly used once during the initial setup of cauldron since these links does not change frequently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo publisher
}) | ||
} | ||
|
||
exports.handler = async function ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting.
|
||
log.debug(`Adding ${mavenUrl ? 'maven' : 'gitHub'}publisher: ${url}`) | ||
|
||
if (!platform || (platform !== 'ios' && platform !== 'android')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically ios
is written as iOS
, comparing platform.toLowerCase() could be a consideration? that would leave the users options to remain case agnostic , so for android.
b462bae
to
7c14141
Compare
@belemaire @krunalsshah Thanks for the review. PR updated with requested changes. |
…aven and GitHub publisher entry in cauldron.
7c14141
to
5c042e3
Compare
No description provided.