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

Avoid block parser warning in SmartyPants #6565

Merged
merged 4 commits into from
Nov 17, 2017
Merged

Conversation

jekyllbot
Copy link
Contributor

PR automatically created for @pathawks.

Avoid block parser warning

@pathawks pathawks changed the title Avoid block parser warning Avoid block parser warning in SmartyPants Nov 16, 2017
@pathawks
Copy link
Member

Huge thanks to @gettalong who wrote the fix for this ❤️:beers:

/cc: @jekyll/build

@pathawks pathawks force-pushed the pull/fix-smartypants branch from 681036d to d462fcf Compare November 16, 2017 21:39
Copy link
Member

@DirtyF DirtyF left a 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.

@DirtyF DirtyF added this to the v3.7.0 milestone Nov 16, 2017
Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

a few inconsistencies..

@span_parsers = [:smart_quotes, :html_entity, :typographic_syms, :span_html]
end

CONTENT_MATCH = %r!^.*\n!
Copy link
Member

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..

Copy link
Member

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?

Copy link
Member

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..

Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

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

Prior discussion: #4323 (comment)

Copy link
Member

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..

def parse_content
add_text(@src.scan(CONTENT_MATCH), @tree)
end
define_parser(:content, %r!^!)
Copy link
Member

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..)

Copy link
Member

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 👍

Copy link
Member

@ashmaroli ashmaroli Nov 17, 2017

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..

Copy link
Member

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

Copy link
Member

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!..

Copy link
Member

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.

@pathawks pathawks dismissed ashmaroli’s stale review November 17, 2017 02:04

I have made changes. Please re-review 👍

@@ -100,6 +100,10 @@ def select; end
assert_equal "“", @filter.smartify("“")
end

should "convert multiple lines" do
assert_equal "…\n…", @filter.smartify("...\n...")
Copy link
Member

Choose a reason for hiding this comment

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

4 \s here....

Copy link
Member

Choose a reason for hiding this comment

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

doh!

@pathawks pathawks force-pushed the pull/fix-smartypants branch from d2cf8a5 to 3f51aba Compare November 17, 2017 02:13
Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@pathawks
Copy link
Member

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 2c646a3 into master Nov 17, 2017
@jekyllbot jekyllbot deleted the pull/fix-smartypants branch November 17, 2017 21:36
jekyllbot added a commit that referenced this pull request Nov 17, 2017
DirtyF pushed a commit that referenced this pull request Dec 7, 2017
DirtyF pushed a commit that referenced this pull request Dec 7, 2017
@jekyll jekyll locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants