remove extra / if passed with --url

[joshgoebel]

No description provided.

[ashmaroli]

@yyyc514 I understand the reason behind this.
Since chomp! will strip only the trailing slash once, --url=http://myblog.tumblr.com// will not be cleansed as expected.

The simplest solution for the moment is to delegate to Ruby's File class:

url = File.join(url, "/api/read/json/")

Additionally, it'd be better if you can add a test for this as well..
Thank you.

[joshgoebel]

HAHA, that's a typo vs an actual expected mistake, but I hear you. Might take me a day or two to circle back but sure we can use File.join and I'll see about a test also.

[joshgoebel]

Better?

[ashmaroli]

@yyyc514 Yes, it is much better now. Thank you.

Technically, your api_feed_url method has not been declared as a private method. But your tests indicate that you intended it to be a private_class_method nonetheless.

So I suggest that you declare it explicitly:

class << self
  # existing methods in this class

  private # declaration towards the end of the class

  def api_feed_url(url, page, per_page: 50)
    url = File.join(url, "/api/read/json/")
    "#{url}?num=#{per_page}&start=#{page * per_page}"
  end
end

The existing methods cannot be declared as private in this PR..

Additionally, I requested the use of File.join due to its simplicity, in comparison to the use of URI.join or Addressable::URI.join. Feel free to use those if they are more intuitive and better in the current context.

[joshgoebel]

jekyll-import/test/test_tumblr_importer.rb

Line 15 in 1ce3a54

assert_equal @posts, Importers::Tumblr.send(:extract_json, @payload)

Why are we using send here? That tripped me up and caused me to not even consider whether it was private or not.

[ashmaroli]

Why are we using send here?

Because at the time when the test was authored, the method was indeed private

But private became redundant in this change and then eventually got removed automatically by RuboCop in subsequent linting sessions because it was "redundant"...

🤷‍♂️

[joshgoebel]

It's redundant because it was outside the class < self, yes? Those methods should still be private as far as I can tell.

[ashmaroli]

Those methods should still be private as far as I can tell.

Possibly. But there's a catch. Exposing private methods as public will be an enhancement. But hiding public methods as private will be a regression (breaking-change).

So please only convert your method to private as I suggested in a couple of comments above.

[joshgoebel]

But hiding public methods as private will be a regression (breaking-change).

Ugh. Indeed. 😐

[ashmaroli]

LGTM! 👍

[joshgoebel]

Finally. 😇

[DirtyF]

@jekyllbot: merge +minor