Improve RSS importer with canonical_link and extract_tags option

[sumanmaity112]

I'll make the required/suggested changes and update the same PR

[ashmaroli]

@sumanmaity112 This also requires tests especially because the implementation expects a particular state. Starting with some basic tests will do.

[sumanmaity112]

@sumanmaity112 This also requires tests especially because the implementation expects a particular state. Starting with some basic tests will do.

Hi @ashmaroli, didn't got your point. Are you asking me to add some sort unit tests?

[ashmaroli]

Hello again, @sumanmaity112 There are couple of issues with the new "private" method you have defined:

• In Ruby, private keyword only affects the instance methods. So, the self.get_tags method is still public. You will have to use the private_class_method :get_tags declaration to make it truly private.
• Please consider refactoring the self.get_tags once more. The following lines do not seem right:

  explicit_tag = options["tag"]
  if explicit_tag
   return explicit_tag unless explicit_tag.nil? || explicit_tag.empty?

  nil is a falsy. The check for explicit_tag.nil? is redundant after the initial if explicit_tag check. Plus, what happened to the exclusivity of the two options? Why ignore options["extract_tags"] if explicit_tag is nil or undefined? (This is why test coverage is important)

[sumanmaity112]

@ashmaroli, refactored the get_tags method. Please do have a look and let me know if I have to update something else

[sumanmaity112]

@ashmaroli, @parkr Any idea how to fix Continuous Integration / Ruby 2.5? It failing to setup the Ruby 2.5 itself

[ashmaroli]

@sumanmaity112 To fix the CI for Ruby 2.5, we will need to lock gem rdoc to an older version. But that is a maintenance task and falls outside the scope of this PR. If you have the time and have the technical know-how regarding this, feel free to submit a pull request. Either ways, I will tackle it in a couple of days.

[parkr]

Locked. If you're comfortable rebasing, please do so!

[sumanmaity112]

Sure, will rebase the changes

[sumanmaity112]

Hi @parkr , I rebased the changes couple days back. Please let me know if anything else is pending from my side which is required to merge this PR.

[ashmaroli]

Hello @sumanmaity112, One final nitpick from my end:

Could we have the CLI option description and docs mirror each other?

I don't see why the concise description in the docs wouldn't be apt for the CLI context display as well.

[sumanmaity112]

sure, will update it

[sumanmaity112]

Hi @parkr, Could you please check if it's better now

[parkr]

@jekyllbot: merge +minor