feat(cli): add --firebase-out flag for custom firebase.json path#435
feat(cli): add --firebase-out flag for custom firebase.json path#435kevmoo wants to merge 2 commits intoinvertase:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a --firebase-out flag to the configure and reconfigure commands, allowing users to specify a custom path for the firebase.json file. The changes involve updating argument parsing, path resolution logic, and adding a corresponding test case. Reviewer feedback suggests that custom paths should be resolved relative to the project root to ensure consistent behavior across different execution environments.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new --firebase-out flag to the configure and reconfigure commands, enabling users to specify a custom path for the firebase.json file. The path resolution logic is now centralized in the FlutterFireCommand base class, and the changes are verified with new integration tests. Feedback highlights the need for defensive programming to handle cases where the Flutter app context might be null and recommends consistent help text for the new flag across different commands.
| String resolveFirebaseJsonPath(String? customPath) { | ||
| if (customPath != null) { | ||
| // If a custom path was provided, check if it is relative. | ||
| if (path.isRelative(customPath)) { | ||
| // Resolve relative paths relative to the project root to ensure consistent behavior. | ||
| return path.join(flutterApp!.package.path, customPath); | ||
| } | ||
| // Use absolute paths as provided. | ||
| return customPath; | ||
| } | ||
| // Default to the standard firebase.json in the project root if no custom path is specified. | ||
| return path.join(flutterApp!.package.path, 'firebase.json'); | ||
| } |
There was a problem hiding this comment.
The resolveFirebaseJsonPath method uses the null-check operator (!) on flutterApp multiple times. While most commands in this CLI require a Flutter app, FlutterFireCommand allows flutterApp to be null. If this method is called when flutterApp is null, it will throw a runtime error. Consider adding a defensive check or using a fallback like Directory.current.path to determine the project root.
| String resolveFirebaseJsonPath(String? customPath) { | |
| if (customPath != null) { | |
| // If a custom path was provided, check if it is relative. | |
| if (path.isRelative(customPath)) { | |
| // Resolve relative paths relative to the project root to ensure consistent behavior. | |
| return path.join(flutterApp!.package.path, customPath); | |
| } | |
| // Use absolute paths as provided. | |
| return customPath; | |
| } | |
| // Default to the standard firebase.json in the project root if no custom path is specified. | |
| return path.join(flutterApp!.package.path, 'firebase.json'); | |
| } | |
| String resolveFirebaseJsonPath(String? customPath) { | |
| final projectRoot = flutterApp?.package.path ?? Directory.current.path; | |
| if (customPath != null) { | |
| // If a custom path was provided, check if it is relative. | |
| if (path.isRelative(customPath)) { | |
| // Resolve relative paths relative to the project root to ensure consistent behavior. | |
| return path.join(projectRoot, customPath); | |
| } | |
| // Use absolute paths as provided. | |
| return customPath; | |
| } | |
| // Default to the standard firebase.json in the project root if no custom path is specified. | |
| return path.join(projectRoot, 'firebase.json'); | |
| } |
| argParser.addOption( | ||
| kFirebaseOutFlag, | ||
| valueHelp: 'filePath', | ||
| help: 'The path to the `firebase.json` file.', | ||
| ); |
There was a problem hiding this comment.
The help text for the firebase-out flag in the Reconfigure command is slightly inconsistent with the one in ConfigCommand. In ConfigCommand, it mentions that the file will be "generated or updated", whereas here it is just "The path to the firebase.json file.". For better user experience, consider using a consistent and descriptive help message across all commands.
| argParser.addOption( | |
| kFirebaseOutFlag, | |
| valueHelp: 'filePath', | |
| help: 'The path to the `firebase.json` file.', | |
| ); | |
| argParser.addOption( | |
| kFirebaseOutFlag, | |
| valueHelp: 'filePath', | |
| help: 'The path to the `firebase.json` file that will be used for reconfiguration.', | |
| ); |
|
"Hi! I noticed the CI tests are failing. |
Fixes #277