wu-sheng commented on code in PR #279:
URL: https://github.com/apache/skywalking-eyes/pull/279#discussion_r3670035619
##########
assets/styles.yaml:
##########
@@ -109,14 +109,14 @@
- id: PythonStyle
# (interpreter binary and encoding comment) | (only interpreter binary) |
(only encoding comment)
- after: '(?m)(^*#!.+$\n^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)|(^*#!.+$)|(^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)'
+ after: '(?m)(^#!.+$\n^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)|(^#!.+$)|(^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)'
start: '#'
middle: '#'
end: '#'
- id: PythonDocStringStyle
# (interpreter binary and encoding comment) | (only interpreter binary) |
(only encoding comment)
- after: '(?m)(^*#!.+$\n^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)|(^*#!.+$)|(^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)'
+ after: '(?m)(^#!.+$\n^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)|(^#!.+$)|(^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)'
Review Comment:
**PythonDocStringStyle** — same fix as above; these two `after` patterns
need to stay in sync.
```suggestion
after: '(?m)(\A#!.+$\n^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)|(\A#!.+$)|(\A[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)'
```
##########
assets/styles.yaml:
##########
@@ -109,14 +109,14 @@
- id: PythonStyle
# (interpreter binary and encoding comment) | (only interpreter binary) |
(only encoding comment)
- after: '(?m)(^*#!.+$\n^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)|(^*#!.+$)|(^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)'
+ after: '(?m)(^#!.+$\n^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)|(^#!.+$)|(^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)'
Review Comment:
**PythonStyle** — `^#!` with `(?m)` still matches the start of **any** line,
and `rewriteContent` uses `FindIndex`, which takes the leftmost match anywhere
in the file. So this narrows the bug rather than fixing it — a `#!` at column 0
inside a mid-file string or comment is still treated as a shebang:
```python
x = """
#!/usr/bin/env python3
# Licensed to the Apache Software Foundation (ASF) under one <-- still
injected mid-string
hello
"""
```
A shebang is only meaningful on line 1, so the anchor should be `\A`. Same
for the encoding branch — PEP 263 only honours a `coding:` declaration on line
1 (line 2 is already covered by the shebang branch above it), but today `^[
\t\f]*#.*?coding[:=]` matches an indented `# coding:` comment at any depth
anywhere in the file.
Keep `(?m)` — it is still needed for the `$` in each branch.
```suggestion
after: '(?m)(\A#!.+$\n^[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)|(\A#!.+$)|(\A[ \t\f]*#.*?coding[:=][
\t]*([-_.a-zA-Z0-9]+).*$)'
```
Verified locally: `go test ./...` passes with this, including the existing
`Python with encoding`, `Python with interpreter binary`, and `Python with
interpreter binary and encoding` cases.
##########
pkg/header/fix_test.go:
##########
@@ -133,6 +133,48 @@ if __name__ == '__main__':
if __name__ == '__main__':
print('Hello World')
+`},
+ {
+ name: "Python with shebang-like string mid file",
+ style: comments.FileCommentStyle("test.py"),
+ content: `def some_function():
+ print(
+ """#!/usr/bin/env python3
+ print("Hello, World!")
+ """)
+`,
+ licenseHeader: getLicenseHeader("test.py", t.Error),
+ expectedContent: `# Apache License 2.0
+# http://www.apache.org/licenses/LICENSE-2.0
+# Apache License 2.0
+
+def some_function():
+ print(
+ """#!/usr/bin/env python3
+ print("Hello, World!")
+ """)
+`},
+ {
+ name: "Python with shebang and shebang-like string mid
file",
+ style: comments.FileCommentStyle("test.py"),
+ content: `#!/usr/bin/env python3
+def some_function():
+ print(
+ """#!/usr/bin/env python3
+ print("Hello, World!")
+ """)
+`,
+ licenseHeader: getLicenseHeader("test.py", t.Error),
+ expectedContent: `#!/usr/bin/env python3
+# Apache License 2.0
+# http://www.apache.org/licenses/LICENSE-2.0
+# Apache License 2.0
+
+def some_function():
+ print(
+ """#!/usr/bin/env python3
+ print("Hello, World!")
+ """)
`},
Review Comment:
Thanks for adding real regression tests — I confirmed they genuinely fail on
`main` (reverting only `assets/styles.yaml` to 6e368ec reproduces the corrupt
output from the PR description).
Both new cases only cover the *indented* variant. Could you also cover the
column-0 variant and a `Hashtag`-style language? Both of these fail on this
branch as-is, and pass once the `\A` anchors are applied:
```suggestion
`},
{
name: "Python with column-0 shebang-like string mid
file",
style: comments.FileCommentStyle("test.py"),
content: `x = """
#!/usr/bin/env python3
hello
"""
`,
licenseHeader: getLicenseHeader("test.py", t.Error),
expectedContent: `# Apache License 2.0
# http://www.apache.org/licenses/LICENSE-2.0
# Apache License 2.0
x = """
#!/usr/bin/env python3
hello
"""
`},
{
name: "Shell with column-0 shebang-like string mid
file",
style: comments.FileCommentStyle("test.sh"),
content: `foo() {
cat <<EOF
#!/bin/bash is just text
EOF
}
`,
licenseHeader: getLicenseHeader("test.sh", t.Error),
expectedContent: `# Apache License 2.0
# http://www.apache.org/licenses/LICENSE-2.0
# Apache License 2.0
foo() {
cat <<EOF
#!/bin/bash is just text
EOF
}
`},
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
