Repository navigation
Conversation
…defaults in databases:create help
…ult)'; --json returns full metadata
Remove false 'Default is DISABLED' claims (omitted settings are left unchanged), list exact allowed values, drop redundant command-local --json option with copy-pasted 'create API call' text (global --json still applies), and name the required flags in the error message.
… in non-interactive mode
There was a problem hiding this comment.
Code Review
This pull request refactors and improves several Firestore CLI commands (databases:delete, databases:get, databases:update, and delete) by enhancing their descriptions, standardizing options (such as using withForce), clarifying error messages, and adding comprehensive unit tests. The review feedback correctly points out that several newly thrown FirebaseError instances for precondition violations should include the { exit: 1 } option to comply with the repository's style guide.
| if (!options.force && options.nonInteractive) { | ||
| throw new FirebaseError( | ||
| `Deleting ${databasePath} requires confirmation. Pass --force to delete the database in non-interactive mode.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
According to the repository style guide, if an error is due to a violation of a precondition (such as requiring confirmation in non-interactive mode), we should specify a non-zero exit code ({ exit: 1 }) in the FirebaseError options.
if (!options.force && options.nonInteractive) {
throw new FirebaseError(
`Deleting ${databasePath} requires confirmation. Pass --force to delete the database in non-interactive mode.`,
{ exit: 1 },
);
}References
- Throw FirebaseError for expected, user-facing errors. If the error is due to a violation of a precondition, specify a non-zero exit code ({ exit: 1 }). (link)
| if (!options.deleteProtection && !options.pointInTimeRecovery) { | ||
| throw new FirebaseError(`Missing properties to update. ${helpCommandText}`); | ||
| throw new FirebaseError( | ||
| `Missing properties to update. Specify at least one of --delete-protection or --point-in-time-recovery. ${helpCommandText}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
According to the repository style guide, if an error is due to a violation of a precondition (such as missing properties to update), we should specify a non-zero exit code ({ exit: 1 }) in the FirebaseError options.
| if (!options.deleteProtection && !options.pointInTimeRecovery) { | |
| throw new FirebaseError(`Missing properties to update. ${helpCommandText}`); | |
| throw new FirebaseError( | |
| `Missing properties to update. Specify at least one of --delete-protection or --point-in-time-recovery. ${helpCommandText}`, | |
| ); | |
| } | |
| if (!options.deleteProtection && !options.pointInTimeRecovery) { | |
| throw new FirebaseError( | |
| `Missing properties to update. Specify at least one of --delete-protection or --point-in-time-recovery. ${helpCommandText}`, | |
| { exit: 1 }, | |
| ); | |
| } |
References
- Throw FirebaseError for expected, user-facing errors. If the error is due to a violation of a precondition, specify a non-zero exit code ({ exit: 1 }). (link)
| if (this.recursive && this.shallow) { | ||
| throw new FirebaseError("Cannot pass recursive and shallow options together."); | ||
| throw new FirebaseError( | ||
| "Cannot pass recursive (-r, --recursive) and shallow (--shallow) options together.", | ||
| ); | ||
| } | ||
|
|
||
| if (this.isCollectionPath && !this.recursive && !this.shallow) { | ||
| throw new FirebaseError("Must pass recursive or shallow option when deleting a collection."); | ||
| throw new FirebaseError( | ||
| "Must pass recursive (-r, --recursive) or shallow (--shallow) option when deleting a collection.", | ||
| ); | ||
| } |
There was a problem hiding this comment.
According to the repository style guide, if an error is due to a violation of a precondition (such as mutually exclusive options or missing required options), we should specify a non-zero exit code ({ exit: 1 }) in the FirebaseError options.
| if (this.recursive && this.shallow) { | |
| throw new FirebaseError("Cannot pass recursive and shallow options together."); | |
| throw new FirebaseError( | |
| "Cannot pass recursive (-r, --recursive) and shallow (--shallow) options together.", | |
| ); | |
| } | |
| if (this.isCollectionPath && !this.recursive && !this.shallow) { | |
| throw new FirebaseError("Must pass recursive or shallow option when deleting a collection."); | |
| throw new FirebaseError( | |
| "Must pass recursive (-r, --recursive) or shallow (--shallow) option when deleting a collection.", | |
| ); | |
| } | |
| if (this.recursive && this.shallow) { | |
| throw new FirebaseError( | |
| "Cannot pass recursive (-r, --recursive) and shallow (--shallow) options together.", | |
| { exit: 1 }, | |
| ); | |
| } | |
| if (this.isCollectionPath && !this.recursive && !this.shallow) { | |
| throw new FirebaseError( | |
| "Must pass recursive (-r, --recursive) or shallow (--shallow) option when deleting a collection.", | |
| { exit: 1 }, | |
| ); | |
| } |
References
- Throw FirebaseError for expected, user-facing errors. If the error is due to a violation of a precondition, specify a non-zero exit code ({ exit: 1 }). (link)
Description
Parent Epic: Buganizer b/581483183 (
Improve agent understanding of CLI behavior)Resolves:
firestore:databases:create)firestore:databases:get)firestore:databases:update)firestore:databases:delete)firestore:delete)Fixes
--force/--non-interactivehandling onfirestore:databases:deleteand clarifies--helpdefaults, output formats, and error messages across Cloud Firestore commands:firestore:databases:create(src/commands/firestore-databases-create.ts): Fixes misleading unconditional"Default is 'ENABLED'"help text on--firestore-data-accessand--realtime-updates, documenting their conditional Enterprise / MongoDB-compatibility defaults accurately.firestore:databases:get(src/commands/firestore-databases-get.ts): Clarifies that[database]defaults to(default)and distinguishes the pretty-printed summary table fields from full metadata returned with--json.firestore:databases:update(src/commands/firestore-databases-update.ts): Removes copy-pasted"create API call"/"Default is DISABLED"option help text (since omitted flags leave existing settings unchanged) and names--delete-protectionand--point-in-time-recoveryin the missing-flag error.firestore:databases:delete(src/commands/firestore-databases-delete.ts): Replaces bespoke--option("--force", ...)and bare-stringconfirm()with standard.withForce()andconfirm({ message, default: false, force, nonInteractive })so-fworks and--non-interactivefails cleanly with remediation instead of an unhandled prompt error.firestore:delete(src/commands/firestore-delete.ts,src/firestore/delete.ts): Clarifies[path]vs.--all-collectionsand-r/--recursivevs.--shallowrequirements in--helpand error messages.Scenarios Tested
npm run buildandnpm run test:compile: Passed with 0 errors.npm run lint:quietandprettier --check: Passed with 0 errors.node_modules/.bin/mocha):src/commands/firestore-databases-update.spec.tssrc/commands/firestore-databases-delete.spec.tssrc/commands/firestore-delete.spec.tsSample Commands