Introduce medium importer #499

sumanmaity112 · 2022-11-28T20:35:22Z

No description provided.

parkr

Minor comments. LGTM

docs/_importers/medium.md

lib/jekyll-import/importers/medium.rb

ashmaroli · 2022-11-29T05:52:40Z

Hello @sumanmaity112,
I am posting a generic comment about my observations instead of a review.

If you take a look at existing importers other than the RSS importer, you'll see that :specify_options definition has a pattern of aligning the various option parameters. I would like you to follow the same alignment style here as well. (In future, I may set up RuboCop to do this for contributors).
You have --canonical_link documented as false by default. But the :process method has it default to true. This needs to be addressed.
I find the description for --render_audio going over my head. What do you mean by enclosure URLs?
We have a Rake task to generate dependency metadata of importers for our docs site. You have to run bundle exec rake site:generate_dependency_data to update the data store for every new importer (and if dependencies for existing importers change). My apology for not having this step documented in our Contribution guide. In future, I plan to have Jekyll automate this for us.
With the above point in mind, if you choose to update to using Importers::RSS.require_deps as @parkr wondered, you have to run the above rake task once again to see if the task is able to register dependencies correctly. If not revert to the original API and update the list as needed.
I suggest using Importers::RSS.process in self.process instead of current RSS.process because it took me some time to realise that you were calling the RSS importer instead of the RSS library from the rss gem.
You have set extract_tags to invariably extract category field info from the RSS feed but have not documented that anywhere. Please leave a Ruby comment above the method for future reference for contributors and maintainers as to why this was done. Additionally, please document a tamer version of the same comment for end-user laymen so that they know what to expect.

sumanmaity112 · 2022-11-29T18:48:06Z

Hi @ashmaroli

I tried to update the importer_dependencies.yml using bundle exec rake site:generate_dependency_data but it started changing dependencies for other importer as well (without any of my changes). I am not sure if it's expected, so for now I manually updated the dependencies for Medium importer.
About the descriptions for --render_audio, it has the same descriptions as mentioned in RSS importer. Please let me know if I have to change it or not. If we want to change then I'll prefer to keep it sync with RSS importer as medium importer just pass the option value to RSS importer.

parkr · 2022-11-29T19:22:44Z

The render_audio documentation was something I asked to be like that. Before it was too vague. Perhaps now it's too specific. This option is basically only useful if you have a podcast RSS feed I think.

sumanmaity112 · 2022-11-29T20:49:31Z

The render_audio documentation was something I asked to be like that. Before it was too vague. Perhaps now it's too specific. This option is basically only useful if you have a podcast RSS feed I think.

Maybe, I am also not sure. This option was there before my changes in RSS importer.

parkr · 2022-11-30T02:33:12Z

lib/jekyll-import/importers/medium.rb

+          "render_audio"   => options.fetch("render_audio", false),
+          "canonical_link" => options.fetch("canonical_link", false),
+          # Add existing tags from Medium post to front matter using `category` subfield on the RSS <item>
+          "extract_tags"   => "category",


Out of curiosity, why not support all of the RSS options? Should we just suggest to users on the docs page for Medium to use the RSS importer and show them how to use it?

Out of curiosity, why not support all of the RSS options?

If you see all the options from RSS importer is there except tag.

Should we just suggest to users on the docs page for Medium to use the RSS importer and show them how to use it?

It's possible but it'll be little verbose. Currently this Medium importer is just the wrapper for RSS importer.

parkr · 2022-11-30T02:33:28Z

lib/jekyll-import/importers/medium.rb

+      def self.process(options)
+        Importers::RSS.process({
+          "source"         => "https://medium.com/feed/@#{options.fetch("username")}",
+          "render_audio"   => options.fetch("render_audio", false),


Do Medium articles typically have audio?

Till now I haven't found any post with audio option. But if I am not wrong, it's possible to add video in medium post

ashmaroli · 2022-11-30T13:25:16Z

Thank you for making the changes as suggested, @sumanmaity112. I have pushed a commit to your branch. Do run git pull origin master before you make additional changes to this proposal.

ashmaroli · 2022-11-30T14:27:42Z

I have some enquiries remaining @sumanmaity112.

Is this importer done being developed? As in, if the RSS importer gets new options, how do we decide if those options need to be ported to the Medium Importer?
It is still not clear to me why you explicitly chose to extract the category field from the feed XML. The Ruby comment you have left needs some more love.
Ideally, we ask contributors to attach tests for pull requests. But given the state of the test-suite for this project, that will be overlooked. Can you instead attach a mock rss file based on your canonical site's export file? You may attach the mock file in the test directory.

sumanmaity112 · 2022-12-05T18:06:24Z

Hi @ashmaroli,

Is this importer done being developed? As in, if the RSS importer gets new options, how do we decide if those options need to be ported to the Medium Importer?

This is depends on the developer. If a new feature is getting added and same feature can be usable in case of Medium import then it's better to add it. According to me, we should have similar options set as RSS import. At the end, Medium import is nothing more than a wrapper around RSS importer with some default configurations.

It is still not clear to me why you explicitly chose to extract the category field from the feed XML. The Ruby comment you have left needs some more love.

I explained the comment little bit more, please do have a look.

Ideally, we ask contributors to attach tests for pull requests. But given the state of the test-suite for this project, that will be overlooked. Can you instead attach a mock rss file based on your canonical site's export file? You may attach the mock file in the test directory.

I added mock RSS feed file generated from my personal Medium feed (only with couple of items). Please let me know if I have to change anything there.

parkr · 2022-12-08T20:35:14Z

Needs a rebase to get @ashmaroli's latest commit to lock Psych to 4.x. I filed a request with ruby/setup-ruby: ruby/setup-ruby#412

sumanmaity112 · 2022-12-10T07:42:36Z

@parkr Rebase is done. Now all CI verification also passed.

parkr · 2022-12-10T20:33:03Z

Thank you!

@jekyllbot: merge +minor

Suman Maity: Introduce medium importer (#499) Merge pull request 499

sumanmaity112 added 2 commits November 28, 2022 21:34

Introduce medium importer

df07fb0

Fix formatting issues

f0c5c87

parkr mentioned this pull request Nov 28, 2022

add importer for Medium articles #445

Closed

parkr approved these changes Nov 28, 2022

View reviewed changes

docs/_importers/medium.md Outdated Show resolved Hide resolved

docs/_importers/medium.md Outdated Show resolved Hide resolved

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

Accomodate PR feedbacks

75a1584

parkr reviewed Nov 30, 2022

View reviewed changes

Sort keys in importer_dependencies.yml data file

403c08b

sumanmaity112 added 2 commits December 5, 2022 18:41

Merge branch 'jekyll:master' into master

602f91b

Accomodate feedback

f3402a3

ashmaroli added 2 commits December 7, 2022 17:40

Move mock feed into test/mocks/

b4f7e4f

Improve clarity of Ruby comment

9b53031

Merge branch 'jekyll:master' into master

7ba0115

jekyllbot merged commit a34ac70 into jekyll:master Dec 10, 2022

jekyllbot added the enhancement label Dec 10, 2022

jekyllbot added a commit that referenced this pull request Dec 10, 2022

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

7bfad93

github-actions bot pushed a commit that referenced this pull request Dec 10, 2022

Deploy docs from a34ac70

ca2375e

Suman Maity: Introduce medium importer (#499) Merge pull request 499

jekyll locked and limited conversation to collaborators Dec 10, 2023

jekyllbot added the frozen-due-to-age label Dec 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce medium importer #499

Introduce medium importer #499

sumanmaity112 commented Nov 28, 2022

parkr left a comment

ashmaroli commented Nov 29, 2022

sumanmaity112 commented Nov 29, 2022

parkr commented Nov 29, 2022

sumanmaity112 commented Nov 29, 2022

parkr Nov 30, 2022

sumanmaity112 Nov 30, 2022

parkr Nov 30, 2022

sumanmaity112 Nov 30, 2022

ashmaroli commented Nov 30, 2022

ashmaroli commented Nov 30, 2022

sumanmaity112 commented Dec 5, 2022

parkr commented Dec 8, 2022

sumanmaity112 commented Dec 10, 2022

parkr commented Dec 10, 2022

Introduce medium importer #499

Introduce medium importer #499

Conversation

sumanmaity112 commented Nov 28, 2022

parkr left a comment

Choose a reason for hiding this comment

ashmaroli commented Nov 29, 2022

sumanmaity112 commented Nov 29, 2022

parkr commented Nov 29, 2022

sumanmaity112 commented Nov 29, 2022

parkr Nov 30, 2022

Choose a reason for hiding this comment

sumanmaity112 Nov 30, 2022

Choose a reason for hiding this comment

parkr Nov 30, 2022

Choose a reason for hiding this comment

sumanmaity112 Nov 30, 2022

Choose a reason for hiding this comment

ashmaroli commented Nov 30, 2022

ashmaroli commented Nov 30, 2022

sumanmaity112 commented Dec 5, 2022

parkr commented Dec 8, 2022

sumanmaity112 commented Dec 10, 2022

parkr commented Dec 10, 2022