search

ysk24ok / jekyll-linkpreview

Jekyll plugin to generate link preview
https://rubygems.org/gems/jekyll-linkpreview
MIT License
30 stars 8 forks source link

fixes & improvements #11

Closed ideas-into-software closed 4 years ago

ideas-into-software commented 4 years ago

Hi,

I just committed few fixes and improvements which I needed to make. Those are:

  1. FIX: file not being closed In lib/jekyll-linkpreview.rb, function save_cache_file had a problem due to file not being closed. This error could be seen on first generation / when no cache entry existed for given URL and attempt to create then read cache entry for given URL was made.

  2. FIX: variable not being extracted properly This error prevented me from passing URLs as variables to the linkpreview tag. Now either string or a variable can be passed with no problems.

  3. IMPROVEMENT: ability to customize layout via template - instead of using those embedded in plugin source code More information about this in code and the updated README.md file.

  4. Other:

    • updated 'spec/jekyll-linkpreview_spec.rb'
    • bumped 'lib/jekyll-linkpreview/version.rb' to version 0.3.0
    • bumped 'jekyll-linkpreview.gemspec' jekyll version to '3.8.6'
    • updated 'README.md'

Some of these fixes and improvements actually coinicide with issues I noticed you had open, which you can now close, i.e.:

Regards, Michael

ysk24ok commented 4 years ago

FIX: variable not being extracted properly This error prevented me from passing URLs as variables to the linkpreview tag.

Would you please add that case to tests ? Because it seems existing test cases passing a variable work properly.

ideas-into-software commented 4 years ago

Hi,

I did update test case you've had ('spec/jekyll-linkpreview_spec.rb') to reflect changes I made.

Unfortunately, I do not have time to create new test cases now - however, if you wish to verify, it's enough to create new Jekyll site (e.g. jekyll new jekyll-linkpreview-testsite), add the plugin (currently released version, i.e. 0.2.0), then attempt to use variables instead of strings, and you'll see how this works, i.e. it does not. Also, you have an existing open issue https://github.com/ysk24ok/jekyll-linkpreview/issues/2 which talks about the same thing, i.e. not being able to use variables in Jekyll sites when using current version of your plugin.

So, to summarize, having this work in an actual deployment (i.e. Jekyll site) is more important than automated test cases which obviously are missing something because the two are not in sync.

Hope this helps.

Michael

ysk24ok commented 4 years ago

It seems jekyll-linkpreview v0.2.0 does work with variables. I tried creating new site by jekyll new, installing v0.2.0 and adding following code to index.md,

{% assign url = 'https://github.com' %}
{% linkpreview url %}

and confirmed it works fine like this.

スクリーンショット 2019-12-13 23 27 19

(the image is so big because no css is applied ...)

I want to know why your attempt to use variables fails. That's why I want you to add a testcase, which fails with v0.2.0 and passes with this pull request. Could you explain your attempt more precisely?

ysk24ok commented 4 years ago

There is a problem (heredoc indent) in this PR, but I'll fix it after merge. Anyway, thank you for this PR!

ideas-into-software commented 4 years ago

Hi, Sorry I could not help with formatting, I did not have the time to check GitHub since then. Thank you for the merge - glad I could help.