-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Avoid block parser warning in SmartyPants #6565
Conversation
Huge thanks to @gettalong who wrote the fix for this ❤️:beers: /cc: @jekyll/build |
681036d
to
d462fcf
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.
LGTM. No more warnings on Jekyll's website.
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.
a few inconsistencies..
lib/jekyll/converters/smartypants.rb
Outdated
@span_parsers = [:smart_quotes, :html_entity, :typographic_syms, :span_html] | ||
end | ||
|
||
CONTENT_MATCH = %r!^.*\n! |
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'd be better to have the constant moved to the top of the class (before :initialize
) like how it is for classes under Jekyll namespace..
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.
Would it be better to just inline it?
I wanted to keep it close to where it is being used. If this class gets too complicated, it should be split out into a separate file. Maybe it's time for that anyway?
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.
If it has to be a regex, then lets inline it with a frozen definition.. (so its still a constant..)
Regarding the move to a new file.., nope.. I'm against it because, this is not a class in our namespace..
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 would live at lib/kramdown/parser/smartypants.rb
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.
Prior discussion: #4323 (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.
true.. but if core is going to consider making the switch to CommonMark
, I feel this abstraction is futile..
lib/jekyll/converters/smartypants.rb
Outdated
def parse_content | ||
add_text(@src.scan(CONTENT_MATCH), @tree) | ||
end | ||
define_parser(:content, %r!^!) |
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.
the regex looks weird to me.. "match something that starts with nothing"...
Also, consistency-wise.. we use regexes with \A
instead of ^
(not that it matters..)
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.
Yeah, it's weird looking, but do you have a better suggestion?
I will use \A
instead of ^
. Thanks for catching that 👍
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'm searching for the source to :define_parser
.. can't seem to find it..
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.
found it.. placing it here for quick reference..
# Add a parser method
#
# * with the given +name+,
# * using +start_re+ as start regexp
# * and, for span parsers, +span_start+ as a String that can be used in a regexp and
# which identifies the starting character(s)
#
# to the registry. The method name is automatically derived from the +name+ or can explicitly
# be set by using the +meth_name+ parameter.
def self.define_parser(name, start_re, span_start = nil, meth_name = "parse_#{name}")
raise "A parser with the name #{name} already exists!" if @@parsers.has_key?(name)
@@parsers[name] = Data.new(name, start_re, span_start, meth_name)
end
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.
k.. I can't come up with a better alternative for the regex.. let's go ahead with %r!\A!
..
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.
We need something that always matches, but never consumes any of the actual string 😆
I think %r!\A!
communicates this as well as anything could, it's just odd.
I have made changes. Please re-review 👍
test/test_filters.rb
Outdated
@@ -100,6 +100,10 @@ def select; end | |||
assert_equal "“", @filter.smartify("“") | |||
end | |||
|
|||
should "convert multiple lines" do | |||
assert_equal "…\n…", @filter.smartify("...\n...") |
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.
4 \s
here....
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.
doh!
d2cf8a5
to
3f51aba
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.
LGTM! 👍
@jekyllbot: merge +bug |
PR automatically created for @pathawks.