Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document is not a Comment. Document shouldn't be assigned to code_object.comment #1210

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tompng
Copy link
Member

@tompng tompng commented Nov 16, 2024

CodeObject#comment is normally String or Comment, but Marshal dump and load creates CodeObject with Document as a comment. So there are two types of CodeObject that shouldn't be mixed:

  • ParsedCodeObject: comment is a String or a Comment
  • MarshalLoadedCodeObject: comment is a Document

This is implicitly doubling the number of total classes in RDoc. It is mixing huge complexity and potential bugs to RDoc codebase. Some method only accepts ParsedCodeObject. Some method only accepts MarshalLoadedCodeObject. It is difficult to know.

def add_extension_modules_single out, store, include
  # include should be RDoc::Markup::(MarshalLoaded)Include. Does not accept RDoc::Markup::(Parsed)Include
  ...
  out << RDoc::Markup::BlankLine.new
  out << include.comment
end

It looks like document is assigned to comment to represent parse-result-cached comment. Alternatively, Comment#document= can be used to represent it.

Ideally, we should avoid mixing String with Comment, but String it is too frequently used. I think it will be easy to remove mixing String after switching to PrismRuby Parser.

I believe we should do this refactor someday but maybe not before Ruby 3.4 because the release date is pretty close.

@st0012 st0012 added the bug label Nov 16, 2024
…ect.comment

CodeObject#comment is normally String or Comment, but Marshal.dump and load creates CodeObject with comment=Document.
Some method requires Document stored in CodeObject#comment, some requires Comment.
We should stop mixing Document with Comment because it is mixing huge complexity and potential bugs to RDoc codebase.
@st0012 st0012 added this to the v7.0.0 milestone Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants