-
Notifications
You must be signed in to change notification settings - Fork 965
Simplified logic for handling "no files" upload notification #729
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: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR simplifies the logic for handling "no files" upload notifications by replacing a switch statement with a lookup table pattern and implementing an early return strategy.
- Replaced switch statement with a constant mapping
NoFileOptionsto core methods - Introduced early exit pattern with
returnstatement to reduce code indentation - Simplified control flow by eliminating the else block after the early return
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/upload/constants.ts | Added NoFileFunctionMap constant mapping enum values to core functions |
| src/upload/upload-artifact.ts | Replaced switch statement with map lookup and implemented early return pattern |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| [NoFileOptions.error]: setFailed, | ||
| [NoFileOptions.ignore]: info, | ||
| [NoFileOptions.warn]: warning | ||
| } as const |
Copilot
AI
Oct 5, 2025
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.
The NoFileFunctionMap could potentially be called with an undefined key if inputs.ifNoFilesFound contains an unexpected value. Consider adding a type annotation or default case to handle this scenario gracefully.
| } as const | |
| } as const | |
| /** | |
| * Safely retrieves the function for the given NoFileOptions value. | |
| * Defaults to warning if the value is unrecognized. | |
| */ | |
| export function getNoFileFunction( | |
| option: string | |
| ): typeof setFailed | typeof info | typeof warning { | |
| if (option === NoFileOptions.error) { | |
| return setFailed; | |
| } else if (option === NoFileOptions.ignore) { | |
| return info; | |
| } else if (option === NoFileOptions.warn) { | |
| return warning; | |
| } else { | |
| // Default to warning for unexpected values | |
| warning(`Unknown if-no-files-found option: ${option}, defaulting to 'warn'.`); | |
| return warning; | |
| } | |
| } |
| NoFileFunctionMap[inputs.ifNoFilesFound]?.( | ||
| `No files were found with the provided path: ${inputs.searchPath}. No artifacts will be uploaded.` | ||
| ) |
Copilot
AI
Oct 5, 2025
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.
Using optional chaining here means that if inputs.ifNoFilesFound is an unexpected value, the function will silently do nothing instead of providing feedback about the invalid option. Consider adding explicit error handling for unsupported values.
| NoFileFunctionMap[inputs.ifNoFilesFound]?.( | |
| `No files were found with the provided path: ${inputs.searchPath}. No artifacts will be uploaded.` | |
| ) | |
| const noFileHandler = NoFileFunctionMap[inputs.ifNoFilesFound]; | |
| if (noFileHandler) { | |
| noFileHandler( | |
| `No files were found with the provided path: ${inputs.searchPath}. No artifacts will be uploaded.` | |
| ); | |
| } else { | |
| core.setFailed( | |
| `Invalid value for 'if-no-files-found': '${inputs.ifNoFilesFound}'. Supported options are: ${Object.keys(NoFileFunctionMap).join(', ')}.` | |
| ); | |
| } |
|
Both of the Copilot suggestions would alter the current functionality. That goes against the current intent. |
|
Changes
NoFileOptionsto their correspondingcoremethodsreturnpattern to further simplify and de-indent the codeEverything after the
returnstatement is just white-space changes.Note
There is no change in functionality here.