-
Notifications
You must be signed in to change notification settings - Fork 441
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
base: master
Are you sure you want to change the base?
Conversation
…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.
523622d
to
7a297e8
Compare
There was a problem hiding this 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.
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? |
There was a problem hiding this comment.
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| |
There was a problem hiding this comment.
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| |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 ❤️
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: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.
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.