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 2 commits 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.
@tompng tompng force-pushed the document_is_not_a_comment branch from 523622d to 7a297e8 Compare November 18, 2024 12:11
@st0012 st0012 added this to the v7.0.0 milestone Nov 18, 2024
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Since this is a potentially risky change and we don't have much time to test it in another release, I'll wait for Ruby 3.4's release before merging it.

Comment on lines 64 to 70
if comment.is_a?(Array)
comment.each do |c|
@comments << extract_comment(c)
end
else
raise TypeError, "unknown comment type: #{comment.inspect}"
comment = extract_comment(comment)
@comments << comment unless comment.empty?
Copy link
Member

Choose a reason for hiding this comment

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

Can it be further simplified to something like:

comments = Array(comment)
comments.each do |c|
  extracted_comment = extract_comment(comment)
  @comments << extracted_comment unless extracted_comment.empty?
end

end
else
raise RDoc::Error, "BUG: unknown comment class #{@comments.class}"
@comments.map do |comment|
Copy link
Member

Choose a reason for hiding this comment

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

nit: @comments.map(&:file)

end
else
raise RDoc::Error, "BUG: unknown comment class #{@comments.class}"
@comments.delete_if do |my_comment|
Copy link
Member

Choose a reason for hiding this comment

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

Can we also rename these locals to something like stored_comment and target_comment?

@comments.delete_if do |stored_comment|
  stored_comment.file == target_comment.file
end

formatter.start_accepting
formatter.accept_document(content)
formatter.end_accepting
comment.text
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for simplifying this part too ❤️

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