Skip to content

Standardize table and heading markup elements #1389

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vinistock
Copy link
Collaborator

@vinistock vinistock commented Jul 12, 2025

I noticed that Markup elements are currently not standardized in any way - even though the code does make some assumptions.

For example, there are visitors that depend on all elements implementing accept, but no parent class or interface that will help guarantee every element implements it. Some elements are implemented as structs, while other are classes. Some have parent classes for shared functionality and APIs and others don't.

I propose that we standardize the markup elements a bit more, so that we can have a common interface for them. It will make it easier for the RDoc to Markdown translator if we can enforce that every markup element has to be able to print itself in Markdown - and I believe the code will be easier to understand too.

This PR creates a base Markup::Element class and I standardized Heading and Table as an example. I'm also finding it challenging to understand the logic with no clue about what types are and what is expected from each method, so I also propose we annotate some of these as documentation - even if we're not adopting type checking.

If folks agree, I will continue to standardize more markup elements. I'm trying to document what each element represents along the way.

@vinistock vinistock requested review from tompng and st0012 July 12, 2025 15:04
@vinistock vinistock self-assigned this Jul 12, 2025
@vinistock vinistock added the refactor A code change that refactors the implementation with no functional changes label Jul 12, 2025
@matzbot
Copy link
Collaborator

matzbot commented Jul 12, 2025

🚀 Preview deployment available at: https://a3db9c6e.rdoc-6cd.pages.dev (commit: e077b2b)

markup = Markup.new
markup.add_regexp_handling CrossReference::CROSSREF_REGEXP, :CROSSREF

@to_html = Markup::ToHtml.new nil
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be a local variable here @to_htmlto_html
It will be assigned to @to_html at @to_html ||= begin

Copy link
Member

@tompng tompng left a comment

Choose a reason for hiding this comment

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

Standardizing markup element classes makes sense 👍

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

Comment on lines +48 to +50
@level = level
@text = text
super()
Copy link
Member

Choose a reason for hiding this comment

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

Initializing the base object (Markup::Element) before this object may be better:

Suggested change
@level = level
@text = text
super()
super()
@level = level
@text = text

Or we may want to remove super() here because the current Markup::Element#initialize is empty.

# HTML markup of the text of this label without the surrounding header element.
#: () -> String
def plain_html
text = self.text.dup
Copy link
Member

Choose a reason for hiding this comment

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

It seems that we don't need dup here.

Comment on lines +82 to +88
text = self.text.dup

if matched = text.match(/rdoc-image:[^:]+:(.*)/)
text = matched[1]
end

self.class.to_html.to_html(text)
Copy link
Member

Choose a reason for hiding this comment

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

How about using more descriptive local variable name and remove self. from self.text something like:

Suggested change
text = self.text.dup
if matched = text.match(/rdoc-image:[^:]+:(.*)/)
text = matched[1]
end
self.class.to_html.to_html(text)
no_image_text = text
if matched = no_image_text.match(/rdoc-image:[^:]+:(.*)/)
no_image_text = matched[1]
end
self.class.to_html.to_html(no_image_text)

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.

I think this is a good idea 👍 But how is this base class compared to RDoc::Markup::Raw? Are they complementary or it could be a replacement?
Also, it may be worth to create a list of elements to update to parallelize/track the work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor A code change that refactors the implementation with no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants