-
Notifications
You must be signed in to change notification settings - Fork 440
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
base: master
Are you sure you want to change the base?
Conversation
🚀 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 |
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.
I think it should be a local variable here @to_html
→ to_html
It will be assigned to @to_html
at @to_html ||= begin
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.
Standardizing markup element classes makes sense 👍
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.
+1
@level = level | ||
@text = text | ||
super() |
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.
Initializing the base object (Markup::Element
) before this object may be better:
@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 |
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.
It seems that we don't need dup
here.
text = self.text.dup | ||
|
||
if matched = text.match(/rdoc-image:[^:]+:(.*)/) | ||
text = matched[1] | ||
end | ||
|
||
self.class.to_html.to_html(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.
How about using more descriptive local variable name and remove self.
from self.text
something like:
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) |
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.
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?
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 standardizedHeading
andTable
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.