Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces logic to normalize the 'only' option in the Command class, ensuring that comma-separated values are correctly handled even when PowerShell replaces commas with spaces. The reviewer suggested removing a debugging console.log statement in favor of the project's logger utility, optimizing the retrieval of the 'only' option, and refactoring the added unit tests into a data-driven format to reduce code duplication.
src/command.ts
Outdated
| if (getInheritedOption(options, "only")) { | ||
| // Handle cases where PowerShell replaces commas with spaces. | ||
| // see https://github.com/firebase/firebase-tools/issues/7506 | ||
| options.only = getInheritedOption(options, "only") | ||
| .split(/[\s,]+/) | ||
| .filter(Boolean) | ||
| .join(","); | ||
| console.log(`--- ${options.only}`); | ||
| } |
There was a problem hiding this comment.
This debugging console.log statement should be removed. According to the repository's style guide, console.log() should not be used for output. If this logging is necessary for debugging, consider using logger.debug() instead.
Additionally, getInheritedOption(options, "only") is called twice. It would be more efficient and readable to call it once and store the result in a variable.
| if (getInheritedOption(options, "only")) { | |
| // Handle cases where PowerShell replaces commas with spaces. | |
| // see https://github.com/firebase/firebase-tools/issues/7506 | |
| options.only = getInheritedOption(options, "only") | |
| .split(/[\s,]+/) | |
| .filter(Boolean) | |
| .join(","); | |
| console.log(`--- ${options.only}`); | |
| } | |
| const only = getInheritedOption(options, "only"); | |
| if (only) { | |
| // Handle cases where PowerShell replaces commas with spaces. | |
| // see https://github.com/firebase/firebase-tools/issues/7506 | |
| options.only = only | |
| .split(/[\s,]+/) | |
| .filter(Boolean) | |
| .join(","); | |
| } |
References
- Use the central
logger(src/logger.ts); never useconsole.log()for user-facing output. (link)
There was a problem hiding this comment.
good catch! Also, missed that console log
| it("should handle comma separated values in 'only' options", async () => { | ||
| const run = command | ||
| .action((options) => { | ||
| return { | ||
| only: options.only, | ||
| }; | ||
| }) | ||
| .runner(); | ||
|
|
||
| const result = await run({ | ||
| only: "firestore,hosting,auth", | ||
| }); | ||
|
|
||
| expect(result).to.deep.eq({ | ||
| only: "firestore,hosting,auth", | ||
| }); | ||
| }); | ||
|
|
||
| it("should normalize space separated values in 'only' options", async () => { | ||
| const run = command | ||
| .action((options) => { | ||
| return { | ||
| only: options.only, | ||
| }; | ||
| }) | ||
| .runner(); | ||
|
|
||
| const result = await run({ | ||
| only: "firestore hosting auth", | ||
| }); | ||
|
|
||
| expect(result).to.deep.eq({ | ||
| only: "firestore,hosting,auth", | ||
| }); | ||
| }); | ||
|
|
||
| it("should normalize space and commas separated values in 'only' options", async () => { | ||
| const run = command | ||
| .action((options) => { | ||
| return { | ||
| only: options.only, | ||
| }; | ||
| }) | ||
| .runner(); | ||
|
|
||
| const result = await run({ | ||
| only: "firestore, hosting, auth", | ||
| }); | ||
|
|
||
| expect(result).to.deep.eq({ | ||
| only: "firestore,hosting,auth", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These tests are great for covering different scenarios for the --only option. However, there's a lot of repeated code. To improve maintainability and make it easier to add new test cases in the future, you could refactor these tests into a data-driven format. This would reduce duplication and make the intent clearer, which aligns with the style guide's emphasis on code quality.
const testCases = [
{
description: "should handle comma separated values in 'only' options",
input: "firestore,hosting,auth",
},
{
description: "should normalize space separated values in 'only' options",
input: "firestore hosting auth",
},
{
description: "should normalize space and commas separated values in 'only' options",
input: "firestore, hosting, auth",
},
];
for (const tc of testCases) {
it(tc.description, async () => {
const run = command
.action((options) => {
return {
only: options.only,
};
})
.runner();
const result = await run({
only: tc.input,
});
expect(result).to.deep.eq({
only: "firestore,hosting,auth",
});
});
}References
- Code should avoid unnecessarily deep nesting or long periods of nesting. Handle edge cases early and exit or fold them into the general case. Consider helper functions that can completely encapsulate branching, e.g. multiple ways a variable can be populated. (link)
There was a problem hiding this comment.
I think the current tests are a bit more readable compared to this? But, I'm open to changing it if this is better
2 participants
Description
Fixes #7506
a somewhat more elegant fix compared to #9878 which does replace all
Scenarios Tested
terminal logs
terminal logs
Sample Commands