Adding possibility to change pagination message by config file

[AlekseySpiridonov]

Default message can by overriten by "paginator_message" parameter in _config.yml

This change allows you to change messages for localize the site to other languages.

[AlekseySpiridonov]

I added information about this change to docs/advanced-usage.md

[AlekseySpiridonov]

I am working on tests and changes for build.
I will update this PR.

[AlekseySpiridonov]

I am working on tests. It takes some time. I will update this PR.

[AlekseySpiridonov]

I have problem with tests for this change (Sorry, I am not Ruby developer. I just backported my own change for my site from local repository, because it looks like helpful for other users).

Looks like I need something like in file spec/jekyll_seo_tag/drop_spec.rb

  context "page_number" do
    let(:paginator) {{ "page" => 2, "total_pages" => 10 }}

    it "check paginator default message" do
      expect(subject.send(:page_number)).to eq("Page 2 of 10 for ")
    end
  end

for testing page_number function.
But it isn't work.

Could you please help\advise with tests for this change?
Thanks.

[ashmaroli]

@AlekseySpiridonov Testing this particular change with RSpec appears to be hard because @context["paginator"] is defined by another plugin (jekyll-paginate) which is not loaded by default. But, if you go through the spec file and the spec_helper file, you'll realize that things are actually quite simple:

  context "pagination" do
    let(:context) do
      make_context(
        { :page => page, :site => site },
        "paginator" => { "page" => 2, "total_pages" => 10 }
      )
    end

    it "renders the correct page number" do
      expect(subject.send(:page_number)).to eq("Page 2 of 10 for ")
    end
  end

The above will pass for the default message. Once that's done, you need to test for the custom message as well:

  context "pagination" do
    let(:context) do
      make_context(
        { :page => page, :site => site },
        "paginator" => { "page" => 2, "total_pages" => 10 }
      )
    end

    it "renders the correct page number" do
      expect(subject.send(:page_number)).to eq("Page 2 of 10 for ")
    end

    # nested context
    context "with custom pagination message" do
      let(:config) { ... }

      it "" do
      end
    end

[AlekseySpiridonov]

@ashmaroli I added tests but it always failed.

Commit - fbecf19

Logs:

Failures:

  1) Jekyll::SeoTag::Drop pagination renders the correct page number
     Failure/Error: paginator_message ||= @site["paginator_message"] || "Page #{current} of #{total} for "
     
     NoMethodError:
       undefined method `[]' for nil:NilClass
     # ./lib/jekyll-seo-tag/drop.rb:192:in `page_number'
     # ./spec/jekyll_seo_tag/drop_spec.rb:494:in `block (3 levels) in <top (required)>'

  2) Jekyll::SeoTag::Drop with custom pagination message renders the correct page number
     Failure/Error: let(:config) {{ "paginator_message" => "#{current} of #{total}" }}
     
     NameError:
       undefined local variable or method `current' for #<RSpec::ExampleGroups::JekyllSeoTagDrop::WithCustomPaginationMessage:0x00007f9f7f2a0d20>
     # ./spec/jekyll_seo_tag/drop_spec.rb:499:in `block (3 levels) in <top (required)>'
     # ./spec/jekyll_seo_tag/drop_spec.rb:7:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:35:in `make_page'
     # ./spec/jekyll_seo_tag/drop_spec.rb:6:in `block (2 levels) in <top (required)>'
     # ./spec/jekyll_seo_tag/drop_spec.rb:8:in `block (2 levels) in <top (required)>'
     # ./spec/jekyll_seo_tag/drop_spec.rb:10:in `block (2 levels) in <top (required)>'
     # ./spec/jekyll_seo_tag/drop_spec.rb:502:in `block (3 levels) in <top (required)>'

Finished in 1.12 seconds (files took 1.18 seconds to load)
202 examples, 2 failures

Could you please check my commit?
Thanks.

[ashmaroli]

@AlekseySpiridonov I knew it would fail. That's the beauty of test-driven-development.
I did not tell it to you beforehand because you would not be able to understand why it would fail..

The fact is that while your implementation works, technically it is not the proper way.
paginator_message.sub("#\{current\}", current.to_s).sub("#\{total\}", total.to_s) is just a hack. Additionally, there is one other mistake in your implementation.

• ||= is not what you think it is doing and is useless in the current context.
• Instead of gsub-ing placeholders, use a proper "string template"

So, the :page_number method ought to be something like:

      def page_number
        return unless @context["paginator"] && @context["paginator"]["page"]

        current = @context["paginator"]["page"]
        total = @context["paginator"]["total_pages"]
        paginator_message = site["paginator_message"] || "Page %<current>s of %<total>s for "

        format(paginator_message, :current => current, :total => total) if current > 1
      end

"Page %<current>s of %<total>s for " is the string-template which will "rendered" by the format() call in the succeeding line.

There's one other teeny mistake in your test as well: your custom-message context is not nested under the default-message context. Perhaps, my example wasn't clear enough.
It should be something like:

context "..." do
  ...
  context "..." do
    ...
  end
end

So, the test will be something like:

context "pagination" do
  let(:context) do
    ...
  end

  it "renders.." do
    ...
  end

  # nested context
  context "with custom message..." do
    let(:config) { { "paginator_message" => "%<current>s of %<total>s" }  }

    it "renders..." do
      ...
    end
  end
end

Now what you have to do is:

• Correct the test
• Correct your implementation
• Correct the documentation asking users to provide the "string template" that contains two variables current and total that will be replaced as necessary.

[AlekseySpiridonov]

@ashmaroli thank you so much.
I changed my PR.

[ashmaroli]

TODO: Merge in master once #325 gets merged

[ashmaroli]

@jekyll/plugin-core Should we nest the paginator_message key under a plugin-specific config key..? If yes, what would the key be..?

[benbalter]

Would it make sense to change the message (in all cases) to "Page ___ / ____" and then just allow "page" to be localized, rather than going down the path of string interpolation? If we are going that route, would a sprintf style %d make more sense/be a cleaner implementation than matching pseduo-ruby in a string?

[ashmaroli]

would a sprintf style %d make more sense/be a cleaner implementation than matching pseduo-ruby in a string?

@benbalter The current approach uses Kernel#format. Isn't that synonymous with / superior over Kernel#sprintf?
Please excuse my naivety, but could you explain how using sprintf would be a cleaner impl.. and what you meant by "matching pseudo-ruby in a string"..??

[benbalter]

@ashmaroli I think I was looking at an outdated diff. Sorry for the noise and thanks for taking the time to further explain. 👍

[DirtyF]

It's nice to be able to localize messages.
I wish every plugin would use a proper namespace though instead of adding another global config key. Maybe we can revisit all current settings under a common key in next major version?

[AlekseySpiridonov]

Do you need some additional changes from my side or we are waiting review?

[berserk24]

I need this change. +1 for merge this pool request.

[DirtyF]

Yay for string translations.

[mattr-]

@jekyllbot: merge +minor

[AlekseySpiridonov]

Thanks a lot!