Add support for Ruby, ERB and slim#266
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comment-style and test coverage to support applying Apache license headers to Ruby (.rb), ERB (.erb / .js.erb), and Slim (.slim) files within the skywalking-eyes header tooling.
Changes:
- Add new comment styles for ERB (
<% ... %>wrapping with#lines) and Slim (single/line comments). - Wire new/updated
comment_style_idmappings for Ruby, ERB-containing languages, and Slim inassets/languages.yaml. - Extend header fixing tests and add include-test fixtures for Ruby/ERB/Slim with and without license headers.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
assets/styles.yaml |
Adds ERB and Slim comment-style definitions used by header generation/fixing. |
assets/languages.yaml |
Maps Ruby / ERB languages and Slim to the new/existing comment styles. |
pkg/header/fix_test.go |
Adds fix/insert expectations for Ruby (with/without shebang), ERB, and Slim. |
test/testdata/include_test/** |
Adds new fixture files for header check tests across Ruby/ERB/Slim. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| start: '/' | ||
| middle: '/' | ||
| end: '/' |
There was a problem hiding this comment.
Is this a a char or a regexp? Others only seem to be chars and not regexp but it fails with a regexp see https://github.com/apache/skywalking-eyes/actions/runs/23291104829/job/67726951492?pr=266#step:6:940 🤷
|
Please fix the CI |
|
@copilot fix the CI |
wu-sheng
left a comment
There was a problem hiding this comment.
I checked out this PR locally to debug the CI failure. There are two separate problems, and fixing only the first one is not enough:
1. The Slash comment style must be literal text, not a regular expression (the inline suggestion below). The style markers are joined literally by GenerateLicenseHeader, which is why the generated headers currently contain a literal ^\/. That's what breaks TestFix/test.slim and TestRewriteContent/Slim*.
2. header check cannot recognize single-/ comments at all, which is why TestCheckFile/WithLicense/.../testcase.slim fails even after reverting to a literal / (this is probably what prompted the ^\/ attempt in 23c1445). The license normalizer in pkg/license/norm.go strips //, /*, */ etc. from headers before matching, but has no rule for a single /, so Slim headers never match the configured license. Please add this rule (it must come after the /* family so it doesn't interfere):
--- a/pkg/license/norm.go
+++ b/pkg/license/norm.go
@@ -52,6 +52,7 @@
regexp.MustCompile(`(?m)^\s*/\*+`), // /*
regexp.MustCompile(`(?m)^\s*\*+/`), // */
regexp.MustCompile(`(?m)^\s*\*+`), // *
+ regexp.MustCompile(`(?m)^\s*/`), // / e.g. Slim, must come after the /* and // patterns
regexp.MustCompile(`(?m)^\s*<!--+`), // <!--
regexp.MustCompile(`(?m)^\s*--+>`), // -->With both changes applied I verified locally on a clean checkout of this branch (and on its merge with current main): the full test suite passes, golangci-lint is clean, and an end-to-end round-trip works — header fix on fresh .slim/.erb/.rb files writes correct headers and header check then validates all of them.
Since "Allow edits by maintainers" is disabled on the fork, I can't push these two changes to this branch myself — please apply the suggestion below and the diff above, and the CI should turn green.
| start: '^\/' | ||
| middle: '^\/' | ||
| end: '^\/' |
There was a problem hiding this comment.
| start: '^\/' | |
| middle: '^\/' | |
| end: '^\/' | |
| start: '/' | |
| middle: '/' | |
| end: '/' |
The comment style markers are literal texts joined by GenerateLicenseHeader, not regular expressions, so these should be the literal /.
There was a problem hiding this comment.
Thanks for troubleshooting it. Just pushed the updated commits.
See apache/skywalking#7744 😅