Skip to content

Stop the generator reading commented-out code - #28

Open
VSN2015 wants to merge 1 commit into
masterfrom
fix/generator-ignores-comments
Open

Stop the generator reading commented-out code#28
VSN2015 wants to merge 1 commit into
masterfrom
fix/generator-ignores-comments

Conversation

@VSN2015

@VSN2015 VSN2015 commented Sep 4, 2026

Copy link
Copy Markdown
Owner

A bug, not a new feature. Found by sweeping the generator's parser against source it will actually meet.

The bug

A controller keeping a line for reference:

def create
  # Legacy: params.require(:admin).permit(:superuser)
  params.require(:user).permit(:name)
end

drafted:

permit_params :create, :update, root: :admin, mode: :monitor do
  optional :superuser, :string, virtual: true # TODO: not a database column
  optional :name, :string # TODO: confirm the type
end

Both :admin and :superuser came from a line that does not execute. The wrong root is annoying; drafting superuser as a permitted parameter is the kind of wrong suggestion that shouldn't survive a review, and shouldn't have been offered in the first place. =begin/=end blocks and trailing comments on live lines had the same effect.

before: {root: :admin, scalars: [:superuser, :name], calls: 2}
after:  {root: :user,  scalars: [:name],             calls: 1}

Why Ripper and not a regexp

# is only sometimes a comment. Two cases a regexp gets wrong, both now pinned by specs:

  • A permit call inside #{...} interpolation is live code and is still read.
  • String content is deliberately kept. permit("name") is a supported spelling and its keys live in string tokens — so dropping string bodies, which is the obvious next step after dropping comments, would silently lose them. That one nearly caught me: the first version of this fix dropped on_tstring_content too.

Ripper is stdlib, so no dependency is added (the gem's one-runtime-dependency claim is untouched, and #14's runtime-deps job would catch it if it weren't).

The fallback

A file Ripper cannot lex falls back to the raw source, so a syntactically odd controller scans exactly as it did before rather than not at all. There's a spec for that too.

Verification

  • 205 examples, 0 failures (6 new, written before the fix): line comments, trailing comments, =begin/=end, a # inside a string, quoted permit keys, and the unlexable fallback
  • RuboCop clean

@VSN2015 VSN2015 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Review of PR #28: Using Ripper lexing to strip comments prevents commented-out parameter code from accidentally drafting root envelopes or fields, while preserving live string literals and interpolations.

# `#{}` interpolation, and a permit call inside interpolation IS live code.
# String CONTENT is deliberately kept: `permit("name")` is a supported
# spelling, and its keys live in string tokens.
COMMENT_TOKENS = %i[on_comment on_embdoc on_embdoc_beg on_embdoc_end].freeze

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Targeting on_comment, on_embdoc, on_embdoc_beg, and on_embdoc_end handles both standard # comments and =begin/=end documentation blocks.

tokens = Ripper.lex(source)
return source if tokens.nil? || tokens.empty?

tokens.reject { |token| COMMENT_TOKENS.include?(token[1]) }.map { |token| token[2] }.join

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Rescuing StandardError and falling back to raw source ensures that files with unusual syntax errors still draft as best as possible rather than failing outright.

A controller keeping a line for reference:

  def create
    # Legacy: params.require(:admin).permit(:superuser)
    params.require(:user).permit(:name)
  end

drafted root: :admin and a :superuser field. Both came from a line that
does not execute, and both are wrong — the second in a
security-flavoured way, since the draft then suggests permitting a
privilege escalation parameter. =begin/=end blocks and trailing
comments on live lines had the same effect.

Comments are removed before scanning, with Ripper rather than a
regexp, because `#` is only sometimes a comment. Two consequences the
regexp approach would get wrong, and which specs now pin:

* A permit call inside #{...} interpolation IS live code, and is still
  read.
* String CONTENT is deliberately kept. permit("name") is a supported
  spelling and its keys live in string tokens, so dropping string
  bodies — the obvious next step — would silently lose them.

Ripper is stdlib, so no dependency is added, and a file it cannot lex
falls back to the raw source: a syntactically odd controller scans
exactly as it did before rather than not at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@VSN2015
VSN2015 force-pushed the fix/generator-ignores-comments branch from 5e4bd12 to 4a5cfea Compare September 11, 2026 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant