feat: add filename validation to newArticles command #289
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @hikagami0210, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the reliability of the newArticles command by implementing filename validation. This change prevents issues arising from invalid file names by checking user input and providing feedback when validation fails.
Highlights
- Filename Validation: The
newArticlescommand now includes validation for filenames, preventing the creation of files with invalid characters or naming conventions. - Error Handling: If a filename fails validation, an error message is displayed, and the creation of that file is skipped, allowing the command to continue processing other valid filenames.
- New Utility and Tests: A new utility file,
src/lib/filename-validator.ts, encapsulates the filename validation logic, with unit tests insrc/lib/filename-validator.test.tsto ensure correctness.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
Adds filename validation to the newArticles command and implements unit tests for the validator
- Introduces
validateFilenameinsrc/lib/filename-validator.tsto check for empty names, forbidden characters, and disallowed start/end characters - Covers validator behavior with Jest tests in
src/lib/filename-validator.test.ts - Hooks the validation into the
newArticlescommand (src/commands/newArticles.ts), logging errors and skipping invalid entries
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/lib/filename-validator.ts | New validateFilename function and result interface, enforcing character rules and trimming |
| src/lib/filename-validator.test.ts | Unit tests verifying valid names, empty inputs, forbidden characters, and edge cases |
| src/commands/newArticles.ts | Apply filename validation before creating articles and report errors via console.error |
Comments suppressed due to low confidence (1)
src/lib/filename-validator.test.ts:5
- Add a test case for filenames containing spaces within the string (e.g., "test file") to ensure internal spaces are correctly handled by the validator.
const validNames = [
| error: "Filename is empty", | ||
| }; | ||
| } | ||
|
|
||
| if (INVALID_FILENAME_CHARS.test(filename)) { | ||
| return { | ||
| isValid: false, | ||
| error: | ||
| 'Filename contains invalid characters: < > : " / \\ | ? * and control characters', |
There was a problem hiding this comment.
[nitpick] Consider exporting these error message strings as constants so both the implementation and tests can reference them, reducing duplication and risk of mismatches when messages change.
| error: "Filename is empty", | |
| }; | |
| } | |
| if (INVALID_FILENAME_CHARS.test(filename)) { | |
| return { | |
| isValid: false, | |
| error: | |
| 'Filename contains invalid characters: < > : " / \\ | ? * and control characters', | |
| error: ERROR_FILENAME_EMPTY, | |
| }; | |
| } | |
| if (INVALID_FILENAME_CHARS.test(filename)) { | |
| return { | |
| isValid: false, | |
| error: ERROR_INVALID_CHARACTERS, |
Copilot uses AI. Check for mistakes.
| @@ -8,6 +9,12 @@ export const newArticles = async (argv: string[]) => { | |||
|
|
|||
| if (args._.length > 0) { | |||
| for (const basename of args._) { | |||
| const validation = validateFilename(basename); | |||
| if (!validation.isValid) { | |||
| console.error(`Error: ${validation.error}`); | |||
There was a problem hiding this comment.
[nitpick] Currently the command skips invalid filenames but always exits with success. Consider setting process.exitCode = 1 when any validation fails so calling processes detect an overall failure.
| console.error(`Error: ${validation.error}`); | |
| console.error(`Error: ${validation.error}`); | |
| hasErrors = true; // Mark that an error occurred |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Code Review
This pull request introduces filename validation for the newArticles command to prevent issues with invalid filenames. The changes include a new filename-validator utility with comprehensive unit tests and its integration into the command. The implementation is solid and well-tested. I have one suggestion to improve code conciseness in the validator.
| if ( | ||
| filename.startsWith(".") || | ||
| filename.endsWith(".") || | ||
| filename.startsWith(" ") || | ||
| filename.endsWith(" ") | ||
| ) { | ||
| return { | ||
| isValid: false, | ||
| error: "Filename cannot start or end with a dot or space", | ||
| }; | ||
| } |
There was a problem hiding this comment.
The check for filenames starting or ending with a dot or space is clear, but a bit verbose using four separate conditions. You can achieve the same result more concisely with a single regular expression. This can improve the code's maintainability.
if (/^[ .]|[ .]$/.test(filename)) {
return {
isValid: false,
error: "Filename cannot start or end with a dot or space",
};
}
What
new (newArticles)コマンドでファイル名のバリデーションを追加しました。また、バリデーションエラー時にエラーメッセージを出力するようにしています。
How
src/commands/newArticles.tsにて、validateFilenameを利用してファイル名のバリデーションを実施console.errorで出力し、処理をスキップWhy
不正なファイル名によるエラーや予期しない挙動を防ぐため、。
Refs