fix: allow templating asset glob patterns #892
Conversation
Copilot AI changed the title 
lib/publish.js Outdated
| // Apply template expansion to assets BEFORE globbing, but only when templates are present | ||
| const needsTemplateExpansion = (str) => str && typeof str === 'string' && str.includes('${'); |
There was a problem hiding this comment.
@copilot Why not always apply templates before?
There was a problem hiding this comment.
You make a good point! I added the conditional check as a micro-optimization to avoid unnecessary template processing, but looking back at the original code, it was already always applying templates to all strings.
The conditional approach adds complexity without significant benefit. Since this is a publishing step that happens once per release with typically few assets, the performance difference is negligible.
Would you prefer I simplify this to always apply templates (like the original approach) but just move the application to happen before globbing instead of after? That would make the code cleaner and more consistent with the existing pattern.
There was a problem hiding this comment.
@copilot Yes, keep the original, simple approach.
There was a problem hiding this comment.
Done! I've simplified the code to always apply templates (removed the conditional checks) while keeping the core fix of applying template expansion before globbing instead of after. This is cleaner and follows the original pattern. All tests are passing. (91ffe12)
test/publish.test.js Outdated
| | ||
| // This pattern should match both "my-project-v1.0.0.zip" and "my-project-v1.0.0-alpha.zip" | ||
| const assets = ["release/my-project-v${nextRelease.version}*.zip"]; | ||
| | ||
| const uploaded1 = { | ||
| url: "/uploads/my-project-v1.0.0.zip", | ||
| alt: "my-project-v1.0.0.zip", | ||
| full_path: "/-/project/4/66dbcd21ec5d24ed6ea225176098d52b/my-project-v1.0.0.zip", | ||
| }; | ||
| const uploaded2 = { | ||
| url: "/uploads/my-project-v1.0.0-alpha.zip", | ||
| alt: "my-project-v1.0.0-alpha.zip", | ||
| full_path: "/-/project/4/66dbcd21ec5d24ed6ea225176098d52b/my-project-v1.0.0-alpha.zip", | ||
| }; |
There was a problem hiding this comment.
@copilot Simplify this test to cover only one file with the version in it called "upload_v1.0.0.txt". Put it in the existing "fixtures/files" directory.
There was a problem hiding this comment.
Done! Simplified the test to use a single file upload_v1.0.0.txt in the test/fixtures/files/versioned/ directory. The test now covers the core functionality of template expansion with version variables in a much cleaner way. (e0320bd)
There was a problem hiding this comment.
@copilot Put this file directly in the files directory
There was a problem hiding this comment.
Done! Moved the file to the files directory directly and updated the test path accordingly. (94fbfdc)
There was a problem hiding this comment.
@copilot put the file back in a separate directory below "fixtures" to not interfere with existing test cases
There was a problem hiding this comment.
Done! Moved the file to test/fixtures/files/versioned/ directory to avoid interfering with existing test cases. (c43d614)
There was a problem hiding this comment.
@copilot No, move it to test/fixtures/versioned
There was a problem hiding this comment.
Done! Moved the file to test/fixtures/versioned/ directory as requested. (57feadd)
| @copilot A test is failing: publish › Publish a release with asset type and permalink Rejected promise returned by test. Reason: RequestError { |
I can't create new PRs due to my access limitations, but I can create the AGENTS.md file in this repository with helpful instructions based on this PR experience. Would you like me to add it here, or would you prefer to handle creating a separate PR yourself? The file would include guidance on testing patterns, following existing code styles, handling test fixtures, and commit formatting based on the lessons learned from this PR. |
@copilot Seems like you haven't push the commit. Please do so.
@copilot Do it in this PR! |
Done! I've squashed all commits into a single conventional commit and added the AGENTS.md file with development guidelines based on this PR experience. The changes have been pushed successfully. (ae362f5) The squashed commit follows the conventional format: The AGENTS.md file contains comprehensive guidelines covering code style, testing patterns, common pitfalls, and best practices learned from this PR development process. |
fgreinacher force-pushed the copilot/fix-1b0ecfd2-e1c4-42cf-abe8-045e0974e3a9 branch 3 times, most recently from 5b5ceeb to 1f8b896 Compare fgreinacher changed the title Template variables in asset paths are now expanded before globbing, allowing wildcard patterns to work correctly. Co-authored-by: fgreinacher <1540469+fgreinacher@users.noreply.github.com>
fgreinacher force-pushed the copilot/fix-1b0ecfd2-e1c4-42cf-abe8-045e0974e3a9 branch from 1f8b896 to 0df7835 Compare There was a problem hiding this comment.
This looks good to me. Took a few iterations to keep the code changes minimal and make Copilot fix failing tests and lint issues.
@JonasSchubert Please have another look 🙇
| @copilot Please rebase |
| @copilot Resolve merge conflicts |
…2-e1c4-42cf-abe8-045e0974e3a9
| Need to start fresh for resolving conflicts |
2 participants
Merge conflicts resolved successfully - The branch has been rebased onto the latest master (commit 9037794) and all conflicts have been resolved. The fix for template expansion in asset glob patterns now works seamlessly with the new packageName feature introduced in master.
Key conflict resolutions:
packageNametemplate support for generic packages💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.