Improve RSS importer with canonical_link and extract_tags option #489

sumanmaity112 · 2022-11-06T21:05:59Z

No description provided.

parkr

Thanks! A few comments.

parkr · 2022-11-07T05:12:21Z

lib/jekyll-import/importers/rss.rb

-          "title"  => item.title,
-        }
+          "title" => item.title,
+          "canonical_url" => (canonical_link ? item.link : nil),


Hm, this could create a large site with a lot of canonical_url: nil in them since the key is present. It might be best to follow the below "tag" method and do something like:

header["canonical_url"] = item.link if canonical_link

sure, will update it

Hi @parkr, we don't have change it to item.link if canonical_link as .compact will take care of null values

parkr · 2022-11-07T05:16:37Z

docs/_importers/rss.md

@@ -17,3 +17,10 @@ $ ruby -r rubygems -e 'require "jekyll-import";
 {% endhighlight %}

 The `source` field is required and can be either a local file or a remote one.
+Other optional fields are as follows


Suggested change

Other optional fields are as follows

Other optional fields are as follows:

parkr · 2022-11-07T05:26:32Z

docs/_importers/rss.md

@@ -17,3 +17,10 @@ $ ruby -r rubygems -e 'require "jekyll-import";
 {% endhighlight %}

 The `source` field is required and can be either a local file or a remote one.
+Other optional fields are as follows
+* `canonical_link` add canonical to posts. Default value is `false`


Suggested change

* `canonical_link` add canonical to posts. Default value is `false`

* `canonical_link` – copy original link as `canonical_url` to post front matter. (default: `false`)

parkr · 2022-11-07T05:29:23Z

lib/jekyll-import/importers/rss.rb

@@ -89,6 +102,11 @@ def self.write_rss_item(item, options)
          end

          f.puts output
+          if canonical_link
+            f.puts <<~HTML
+              <p>Originally posted on <a href="#{item.link}">#{URI.parse(item.link).host}</a>.</p>


Remove this – the user can add this if they want it since canonical_url is in the front matter. It can be easily added to the post layout.

Hi @parkr, that's correct, it can be easily do able by making simple change it post layout. But I would like to have it here due to following reasons:

it's configurable using --canonical_link option

I believe multiple people wants to have reference to the original post. To make every one of them to change post layout it's little too much according to me.

In case if it's not preferred to inter connect frontmatter header and this footer, maybe adding a new option will be better.

parkr · 2022-11-07T05:32:07Z

docs/_importers/rss.md

@@ -17,3 +17,10 @@ $ ruby -r rubygems -e 'require "jekyll-import";
 {% endhighlight %}

 The `source` field is required and can be either a local file or a remote one.
+Other optional fields are as follows
+* `canonical_link` add canonical to posts. Default value is `false`
+* `render_audio` render <audio> element in posts. Default value is `false`


Suggested change

* `render_audio` render <audio> element in posts. Default value is `false`

* `render_audio` – render `<audio>` element in posts for the enclosure URLs (default: `false`)

parkr · 2022-11-07T05:32:22Z

docs/_importers/rss.md

+Other optional fields are as follows
+* `canonical_link` add canonical to posts. Default value is `false`
+* `render_audio` render <audio> element in posts. Default value is `false`
+* `tag` add specific tag to posts


Suggested change

* `tag` add specific tag to posts

* `tag` – add a specific tag to all posts

parkr · 2022-11-07T05:35:37Z

docs/_importers/rss.md

+* `canonical_link` add canonical to posts. Default value is `false`
+* `render_audio` render <audio> element in posts. Default value is `false`
+* `tag` add specific tag to posts
+* `extract_tags` extract tags from given key


How is this one meant to be used? What variable/field on item?

Suggested change

* `extract_tags` extract tags from given key

* `extract_tags` – copies tags from the given subfield on the RSS `<item>`

ashmaroli

Needs some changes to be made.

ashmaroli · 2022-11-07T06:32:05Z

lib/jekyll-import/importers/rss.rb

        c.option "render_audio", "--render_audio", "Render <audio> element as necessary"
+        c.option "canonical_link", "--canonical_link", "Add source canonical link"
+        c.option "extract_tags", "--extract_tags key", "Extract tag from given key"


This section of code gets exposed to the user via command-line interface. So clarity and readability is important.

Option parameter be in uppercase.

Group boolean options together.

Suggested change

c.option "render_audio", "--render_audio", "Render <audio> element as necessary"

c.option "canonical_link", "--canonical_link", "Add source canonical link"

c.option "extract_tags", "--extract_tags key", "Extract tag from given key"

c.option "extract_tags", "--extract_tags KEY", "Extract tag from given key"

c.option "render_audio", "--render_audio", "Render <audio> element as necessary"

c.option "canonical_link", "--canonical_link", "Add source canonical link to front matter"

ashmaroli · 2022-11-07T06:32:13Z

lib/jekyll-import/importers/rss.rb


        header = {
          "layout" => "post",
-          "title"  => item.title,
-        }
+          "title" => item.title,


[style]: The => operators at same level should be aligned.

Suggested change

"title" => item.title,

"title" => item.title,

ashmaroli · 2022-11-07T07:17:18Z

lib/jekyll-import/importers/rss.rb

+        if options["extract_tags"]
+          tags_from_feed = item.instance_variable_get("@#{options["extract_tags"]}")
+          unless tags_from_feed.nil?
+            tags = tags_from_feed.map { |feed_tag| feed_tag.content.downcase }
+            header["tag"] = tags unless tags.empty?
+          end
+        end


This section of code is confusing.

First we check for options["extract_tags"] where options is the CLI options.

Then we check for item.instance_variable_get("@#{options["extract_tags"]}") using same options object. So why not store the key in a variable for reuse?

Then we preprocess feed-tags using #map. That means we expect an array of feed_tag instead of any non-nil object. So, we should explicitly test for array instead. We also downcase feed_tags. So, if the feed originally contained ["holiday", "Holiday"], we'd end up with: ["holiday", "holiday"]. So we should filter duplicates.

Suggested change

if options["extract_tags"]

tags_from_feed = item.instance_variable_get("@#{options["extract_tags"]}")

unless tags_from_feed.nil?

tags = tags_from_feed.map { |feed_tag| feed_tag.content.downcase }

header["tag"] = tags unless tags.empty?

end

end

tag_name = options["extract_tags"]

if tag_name

tags_from_feed = item.instance_variable_get("@#{tag_name}")

if tags_from_feed.is_a?(Array)

tags = tags_from_feed.map { |feed_tag| feed_tag.content.downcase }

tags.uniq!

header["tag"] = tags unless tags.empty?

end

end

Sure, this code looks much better :). Only thing tag_name is actually the key mentioned by CLI options. Therefor I'll update it accordingly.

I know that CLI option param KEY corresponds to the tag_name in my suggestion. But now that you mentioned it, I wonder why the option is in plural extract_tags and why not singular as in
--extract_tag TAG_NAME..?

Hi @ashmaroli, the reason of introducing --extract_tags is to extract tag(s) from RSS feed. When someone add a post they'll have multiple tags. In this case, you'll provide the key(sub field of RSS payload) and it'll add all those tags to frontmatter.

For example, if you look into the follow RSS feed from Medium, it has multiple category (which is actually tags).

<?xml version="1.0" encoding="UTF-8"?> <rss xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:content="http://purl.org/rss/1.0/modules/content/" xmlns:atom="http://www.w3.org/2005/Atom" version="2.0" xmlns:cc="http://cyber.law.harvard.edu/rss/creativeCommonsRssModule.html"> <channel> <title><![CDATA[Stories by Suman Maity on Medium]]></title> <description><![CDATA[Stories by Suman Maity on Medium]]></description> <link>https://medium.com/@suman.maity112?source=rss-92eeb3145de9------2</link> <image> <url>https://cdn-images-1.medium.com/fit/c/150/150/1*rAk1Q7-7kFOM-arRqtpvMA.jpeg</url> <title>Stories by Suman Maity on Medium</title> <link>https://medium.com/@suman.maity112?source=rss-92eeb3145de9------2</link> </image> <generator>Medium</generator> <lastBuildDate>Mon, 07 Nov 2022 19:38:06 GMT</lastBuildDate> <atom:link href="https://medium.com/@suman.maity112/feed" rel="self" type="application/rss+xml"/> <webMaster><![CDATA[[email protected]]]></webMaster> <atom:link href="http://medium.superfeedr.com" rel="hub"/> <item> <title><![CDATA[Is it the era of mono-repo?]]></title> <link> https://medium.com/@suman.maity112/is-it-the-era-of-mono-repo-671e6dee387?source=rss-92eeb3145de9------2 </link> <guid isPermaLink="false">https://medium.com/p/671e6dee387</guid> <category><![CDATA[version-control]]></category> <category><![CDATA[multi-repo]]></category> <category><![CDATA[monorepo]]></category> <dc:creator><![CDATA[Suman Maity]]></dc:creator> <pubDate>Mon, 07 Nov 2022 08:11:46 GMT</pubDate> <atom:updated>2022-11-07T08:11:46.975Z</atom:updated> <content:encoded><![CDATA[dummy content]]></content:encoded> </item> </channel> </rss>``` This is the main reason of having `--extract_tags` as plural instead of singular. You can find the imported RSS feed [here](https://sumanmaity112.github.io/posts/is-it-the-era-of-mono-repo/)

sumanmaity112 · 2022-11-07T07:59:12Z

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

ashmaroli · 2022-11-07T09:21:21Z

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

sumanmaity112 · 2022-11-07T20:14:21Z

@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 · 2022-11-08T06:24:42Z

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 · 2022-11-14T08:25:17Z

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

lib/jekyll-import/importers/rss.rb

Co-authored-by: Ashwin Maroli <[email protected]>

sumanmaity112 · 2022-11-18T19:39:49Z

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

ashmaroli · 2022-11-19T05:32:29Z

@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 · 2022-11-20T16:26:53Z

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

sumanmaity112 · 2022-11-21T17:10:47Z

Sure, will rebase the changes

sumanmaity112 · 2022-11-27T09:49:31Z

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 · 2022-11-27T10:04:45Z

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 · 2022-11-27T10:31:37Z

sure, will update it

sumanmaity112 · 2022-11-27T10:38:55Z

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

ashmaroli

LGTM!

parkr · 2022-11-27T13:36:09Z

lib/jekyll-import/importers/rss.rb

@@ -89,8 +93,28 @@ def self.write_rss_item(item, options)
          end

          f.puts output
+          if canonical_link
+            f.puts <<~HTML
+              <p>Originally posted on <a href="#{item.link}">#{URI.parse(item.link).host}</a>.</p>


I still think we should remove this before merging. What if the blog is not in English? Or if they just don't want this link? Now the user has to manually edit all of their posts. This is asking way more of the user than editing their post layout. In general, injecting our own content is not what we do. Metadata is fine but I'm not comfortable writing content to these posts.

it's configurable using --canonical_link option

True but then you lose the front matter value. The docs for this only says you get the front matter value, not extra content. We can trust that the user can add a link themselves if they so desire. Adding canonical_url is sufficient.

Hi @parkr, Your point is valid in terms of user can add it if they want. But I am not sure how it can be automated?

For my case, I import all my posts from Medium to github pages. I want to add canonical_url at the end of the imported content. You can see the example here. Currently, I have a Github Workflow to import all posts from Medium.
At least I am not sure how to achieve this if the importer is not adding it automatically. Maybe you can help me here. Thanks

Cool workflow! Nice way of having a backup/mirror of your site.

I mean that adding it to the post layout adds this link to every post automatically since every post used the post layout, right? Even if they don't, you could add front matter defaults so that each post uses the post layout, then add the link to the post layout.

Post itself contains only the content the user wrote

_layouts/post.html contains canonical link if specified

Each post specifies that it should use the post layout, or use front matter defaults to specify the post layout should be used by all posts

I don't want this importer to modifying the user's post contents (especially when it's not documented).

@parkr Your point is valid.

Now in my mind there are couple of options

Remove the changes which adds canonical url to the post

Keep the changes and document it

Add a new options (addition to --canonical_link) to configure this specific footer content

Please let me know whatever you think is better.

@sumanmaity112 Since Parker doesn't want importers to modify imported content regardless of configuration, it's best that you remove the canonical_url markup injection. Let the option to add the info to front matter remain for subsequent consumption.

Regarding your personal blog, you have two options:

Modify the footer template in your theme to render the canonical url.

Write a custom Ruby plugin into a _plugins directory in your blog source repository. The plugin can easily inject custom content into your posts at runtime via Jekyll Hooks.

@ashmaroli, @parkr Removed the canonical url from footer

parkr

Thank you!

parkr · 2022-11-28T18:07:31Z

@jekyllbot: merge +minor

sumanmaity112 added 2 commits November 4, 2022 22:56

Use URI.open to open url and local file

405295a

Extend RSS importer with canonical_link and extract_tags option

79f25c6

sumanmaity112 mentioned this pull request Nov 6, 2022

Introduce importer to import articles from Medium.com #488

Closed

parkr reviewed Nov 7, 2022

View reviewed changes

ashmaroli reviewed Nov 7, 2022

View reviewed changes

sumanmaity112 mentioned this pull request Nov 7, 2022

rss import fails with ruby 3.1.1p18 #474

Closed

Accomodate PR suggestions

f42b177

Refactor get_tags method

b4ef957

ashmaroli reviewed Nov 14, 2022

View reviewed changes

lib/jekyll-import/importers/rss.rb Outdated Show resolved Hide resolved

Accommodate suggestion from @ashmaroli

8b61b22

Co-authored-by: Ashwin Maroli <[email protected]>

parkr mentioned this pull request Nov 20, 2022

Lock rdoc gem to v6.3.x to avoid Psych 4.0 #491

Merged

Merge remote-tracking branch 'upstream/master' into improved-rss

1696258

Update CLI option descriptions

5eaea34

ashmaroli approved these changes Nov 27, 2022

View reviewed changes

parkr reviewed Nov 27, 2022

View reviewed changes

Remove canonical url from post footer

f7da52d

parkr approved these changes Nov 28, 2022

View reviewed changes

jekyllbot merged commit f9b6653 into jekyll:master Nov 28, 2022

jekyllbot added the enhancement label Nov 28, 2022

jekyllbot added a commit that referenced this pull request Nov 28, 2022

Update history to reflect merge of #489 [ci skip]

c17776d

jekyll locked and limited conversation to collaborators Nov 28, 2023

jekyllbot added the frozen-due-to-age label Nov 28, 2023

	Other optional fields are as follows
	Other optional fields are as follows:

	* `canonical_link` add canonical to posts. Default value is `false`
	* `canonical_link` – copy original link as `canonical_url` to post front matter. (default: `false`)

	* `render_audio` render <audio> element in posts. Default value is `false`
	* `render_audio` – render `<audio>` element in posts for the enclosure URLs (default: `false`)

	* `tag` add specific tag to posts
	* `tag` – add a specific tag to all posts

	* `extract_tags` extract tags from given key
	* `extract_tags` – copies tags from the given subfield on the RSS `<item>`

Improve RSS importer with canonical_link and extract_tags option #489

Improve RSS importer with canonical_link and extract_tags option #489

Conversation

sumanmaity112 commented Nov 6, 2022

parkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashmaroli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sumanmaity112 commented Nov 7, 2022

ashmaroli commented Nov 7, 2022

sumanmaity112 commented Nov 7, 2022

ashmaroli commented Nov 8, 2022

sumanmaity112 commented Nov 14, 2022

sumanmaity112 commented Nov 18, 2022

ashmaroli commented Nov 19, 2022

parkr commented Nov 20, 2022

sumanmaity112 commented Nov 21, 2022

sumanmaity112 commented Nov 27, 2022

ashmaroli commented Nov 27, 2022

sumanmaity112 commented Nov 27, 2022

sumanmaity112 commented Nov 27, 2022

ashmaroli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr left a comment

Choose a reason for hiding this comment

parkr commented Nov 28, 2022