Closed sumanmaity112 closed 1 year ago
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.
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.
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.
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.
I have some enquiries remaining @sumanmaity112.
category
field from the feed XML. The Ruby comment you have left needs some more love.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.
Needs a rebase to get @ashmaroli's latest commit to lock Psych to 4.x. I filed a request with ruby/setup-ruby: https://github.com/ruby/setup-ruby/issues/412
@parkr Rebase is done. Now all CI verification also passed.
Thank you!
@jekyllbot: merge +minor
Hello @sumanmaity112, I am posting a generic comment about my observations instead of a review.
: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).--canonical_link
documented asfalse
by default. But the:process
method has it default totrue
. This needs to be addressed.--render_audio
going over my head. What do you mean by enclosure URLs?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.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.Importers::RSS.process
inself.process
instead of currentRSS.process
because it took me some time to realise that you were calling the RSS importer instead of the RSS library from therss
gem.extract_tags
to invariably extractcategory
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.