-
Notifications
You must be signed in to change notification settings - Fork 109
Extract sanitizeWorkflowName to dedicated helper module
#3144
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
Conversation
- Created pkg/workflow/js/sanitize_workflow_name.cjs with the function - Updated parse_firewall_logs.cjs to import from new helper - Removed function definition from parse_firewall_logs.cjs - Updated parse_firewall_logs.test.cjs to remove sanitizeWorkflowName test - Created sanitize_workflow_name.test.cjs with comprehensive tests - All tests passing (655 tests, 34 test files) - TypeScript typecheck passing - Code formatted with prettier Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
sanitizeWorkflowName to dedicated helper module
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 extracts the sanitizeWorkflowName function from parse_firewall_logs.cjs into its own dedicated module with comprehensive test coverage. This refactoring improves code organization and reusability.
- Moved
sanitizeWorkflowNamefunction to a new standalone modulesanitize_workflow_name.cjs - Created comprehensive test suite for the extracted function
- Updated
parse_firewall_logs.cjsto import the function instead of defining it locally
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pkg/workflow/js/sanitize_workflow_name.cjs |
New module containing the extracted sanitizeWorkflowName function |
pkg/workflow/js/sanitize_workflow_name.test.cjs |
New comprehensive test suite for sanitizeWorkflowName |
pkg/workflow/js/parse_firewall_logs.cjs |
Removed sanitizeWorkflowName function and its export, added import from new module |
pkg/workflow/js/parse_firewall_logs.test.cjs |
Removed test for sanitizeWorkflowName and cleanup of test setup code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot merge main and recompile |
…ize-workflow-name
- Merged latest changes from main branch - Recompiled all workflow lock files - All unit tests passing (Go + JavaScript) - Lock files updated to reflect latest changes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot the require is not properly inlined by the bundled. Update compiler to embed new cjs file and bundle properly |
- Added embed directive for sanitize_workflow_name.cjs - Registered sanitize_workflow_name.cjs in GetJavaScriptSources() - Converted parse_firewall_logs.cjs to use bundling pattern - Added getParseFirewallLogsScript() with lazy bundling - Updated GetLogParserScript() to use bundled version - Verified bundler properly inlines sanitize_workflow_name function - All tests passing (Go + JavaScript) - Recompiled all workflow lock files Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated compiler to properly bundle sanitize_workflow_name.cjs in commit e4ff745. The bundler now:
Verified bundler correctly inlines the function and removes exports. |
|
@copilot merge main |
…ize-workflow-name # Conflicts: # .github/workflows/smoke-copilot.firewall.lock.yml
pkg/workflow/js/sanitize_workflow_name.cjswith the functionpkg/workflow/js/parse_firewall_logs.cjsto import from new helperparse_firewall_logs.cjsOriginal prompt
sanitizeWorkflowNameto dedicated helper file #3134✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.