-
Notifications
You must be signed in to change notification settings - Fork 458
issue-1060 Add scriptId argument to list-deployments and list-versions commands #1066
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
5c46ac9
to
143f6d1
Compare
Didn't mean to close the issue yet, but wanted the --json flag to be a global option so it's consistent throughout all the commands. If you can resolve the merge conflicts, happy to merge the parameter stuff (but similarly, would rather do this uniformly if there are additional commands that would benefit) |
Ah okay I thought you were closing it because you were already implementing it globally as a broader change and this was unnecessary. |
Also should be able to do this without modifying anything in src/core. There's a method on the clasp instance to set the script ID if it isn't already set:
Right now that |
Allow script ID as argument for list-deployments & list-versions Add --json option to list-deployments & list-versions Updated commands
The intention behind that change was to be able to run it with the script id of library dependencies, not limited to the root package. When our users are trying to manage a package that has several libraries they have to navigate to the project and click on each package there. Do you have any thoughts on that implementation or is it something that is out of scope for the project? |
This is what I was thinking to relax the check. withScriptId(scriptId: string) {
if (this.options.project) {
debug('Project is already configured, overriding scriptId with %s', scriptId);
}
this.options.project = {
scriptId,
};
return this;
} |
Relax the withScriptId to allow scriptId input inside a configured project Use global --json instead of command level
143f6d1
to
8d8f0d2
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.
Added a few comments, mostly just to simplify this further.
Also please add test cases
src/commands/list-deployments.ts
Outdated
const description = d.deploymentConfig?.description ? `- ${d.deploymentConfig.description}` : ''; | ||
console.log(`- ${d.deploymentId} ${versionString} ${description}`); | ||
}); | ||
if (options?.json) { |
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.
This is unnecessary now. The --json output is already handled above (~ line 44) and it returns, so this code path should never run.
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.
Done
src/commands/list-deployments.ts
Outdated
const deployments = await withSpinner(spinnerMsg, async () => { | ||
const deployments = await withSpinner(spinnerMsg, () => { | ||
// If a scriptId is provided, set it on the clasp instance. | ||
if (scriptId) { |
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.
Minor nit, suggest moving up to after const clasp...
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.
Moved
src/commands/list-versions.ts
Outdated
); | ||
console.log(msg); | ||
}); | ||
if (options?.json) { |
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.
Same as other file, this shouldn't be necessary or ever execute since the JSON flag is checked earlier
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.
Removed
src/commands/list-versions.ts
Outdated
const versions = await withSpinner(spinnerMsg, async () => { | ||
const versions = await withSpinner(spinnerMsg, () => { | ||
// If a scriptId is provided, set it on the clasp instance. | ||
if (scriptId) { |
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.
Minor nit, same as before (move to after the const clasp...
line)
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.
Moved
README.md
Outdated
@@ -460,9 +461,16 @@ Clasp offers several commands to opens the current directory's `clasp` project a | |||
|
|||
List deployments of a script. | |||
|
|||
#### Options |
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 forgot to document this as a global flag in the last change, so good reminder :)
Can you move this up to the global options section please? Same for list versions too
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.
Done
@@ -531,9 +539,16 @@ Creates an immutable version of the script. | |||
|
|||
List versions of a script. | |||
|
|||
#### Options |
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.
Same as other, can/should be consolidated under global options
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.
Done
Add test cases
Allow script ID as argument for list-deployments & list-versions
Add --json option to list-deployments & list-versions
Fixes #<issue_number_goes_here> (it's a good idea to open an issue first for discussion)
npm run test
succeeds.npm run lint
succeeds.