Skip to content

fix: avoid false positives in ban-ts-comment#607

Merged
fansenze merged 1 commit intomainfrom
fix/ts_comment
Apr 7, 2026
Merged

fix: avoid false positives in ban-ts-comment#607
fansenze merged 1 commit intomainfrom
fix/ts_comment

Conversation

@simplyme0823
Copy link
Copy Markdown
Contributor

Summary

Related Links

avoid false positives in ban-ts-comment, e.g const c = '// @ts-ignore'

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the ban_ts_comment rule to use the TypeScript scanner via utils.ForEachComment instead of manual string parsing, ensuring that comment-like text within strings or template literals is correctly ignored. While the logic improvement is sound, several critical issues were identified regarding string offset handling. The current implementation incorrectly slices UTF-8 Go strings using UTF-16 offsets from the TypeScript shim, which will cause failures with non-ASCII characters. Furthermore, there are inconsistencies in bounds checking and diagnostic reporting due to mixing UTF-16 and byte-based units, and the availability of the ForEachComment utility needs to be verified.

Comment thread internal/plugins/typescript/rules/ban_ts_comment/ban_ts_comment.go
Comment thread internal/plugins/typescript/rules/ban_ts_comment/ban_ts_comment.go
Comment thread internal/plugins/typescript/rules/ban_ts_comment/ban_ts_comment.go
Comment thread internal/plugins/typescript/rules/ban_ts_comment/ban_ts_comment.go
@fansenze fansenze merged commit 6032cd5 into main Apr 7, 2026
11 checks passed
@fansenze fansenze deleted the fix/ts_comment branch April 7, 2026 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants