8000 Expose `basename` from `document.rb` as `name` to Liquid templates by jjnilton · Pull Request #8761 · jekyll/jekyll · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Expose basename from document.rb as name to Liquid templates #8761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Apr 1, 2022
Merged

Expose basename from document.rb as name to Liquid templates #8761

merged 8 commits into from
Apr 1, 2022

Conversation

jjnilton
Copy link
Contributor
@jjnilton jjnilton commented Aug 8, 2021 •
edited by ashmaroli
Loading
  • This is a 🐛 bug fix.
  • The test suite passes locally.

Summary

Expose basename method from document.rb as name in Liquid (document_drop.rb), and add basename method to excerpt.rb, preventing error/failure in test.

Context

Fixes #8699.

jjnilton added 2 commits August 7, 2021 18:30
@jjnilton
Update document_drop.rb
d73e388 
Expose `basename` method as `name` in Liquid, so `{{ page.name }}` works in posts.
@jjnilton
Update excerpt.rb
3143865 
Add `basename` method to `excerpt` class.
@jjnilton jjnilton changed the title Expose basename as name liquid Expose basename from document.rb as name Liquid (document_drop.rb), enabling {{ page.name }} in posts/pages Aug 8, 2021
@jjnilton jjnilton mentioned this pull request Aug 8, 2021
Closed
@ashmaroli
Copy link
Member

@jjnilton This needs a test coverage as well.
Moreover, why is the excerpt drop method basename instead of name?

@jjnilton
Copy link
Contributor Author

@ashmaroli sorry for taking so long to reply here.

I think I will need some help with the tests. I searched the code to find some test examples with the path and relative_path methods and seems that this needs tests in test_excerpt and test_filters, but I'm not sure.

My thinking was that excerpt should be like the document, so the methods have the same name, and since excerpt_drop inherits from document_drop, both would have the basename and name.

@ashmaroli ashmaroli changed the title Expose basename from document.rb as name Liquid (document_drop.rb), enabling {{ page.name }} in posts/pages Expose basename from document.rb as name to Liquid templates Aug 18, 2021
@ashmaroli
Copy link
Member

@jjnilton First of all DocumentDrop instances do not respond to :basename, only Document instances do.
With your change, DocumentDrop instances will now respond to :name but still not :basename (because of the delegation). Therefore, ExcerptDrop instances need not respond to :basename as well.

Hope I was able to clear that fog.

Regarding tests, since you are only improving exposed Liquid attributes for documents (also posts), the best location to add a test would be in features/post_data.feature. Follow the pattern used in existing scenarios in that file and add a new scenario to test if using {{ page.name }} produces the expected results and resolves your concern in #8699

@ashmaroli
Copy link
Member

ping @jjnilton

@jjnilton
Copy link
Contributor Author
jjnilton commented Mar 6, 2022

@ashmaroli sorry for taking so long to reply (again), and thanks for the explanations.

With your change, DocumentDrop instances will now respond to :name but still not :basename (because of the delegation)
Therefore, ExcerptDrop instances need not respond to :basename as well.

I changed the excerpt.rb part to this:

    # The base filename of the excerpt.
    #
    # Returns the base filename for the doc this excerpt belongs.
    def basename
      @basename ||= File.join(doc.basename, "#excerpt")
    end

And it does not return the basename in the exercepts with {{ page.basename }}, only with {{ page.name}}.

Adding to document_drob.rb makes it return for both.:

      delegate_methods :id, :output, :content, :to_s, :relative_path, :url, :date, :basename

If I don't use basename for name of the method in excerpt.rb I get this error when trying to run jekyll:

jekyll/lib/jekyll/drops/drop.rb:72:in block in delegate_method_as': undefined method basename' for #Jekyll::Excerpt:0x000055d786320c18 (NoMethodError)

I think that I've slowed down this fix, so if you or someone else want continue, please do. Maybe it'll help me understand how it should be done.

ashmaroli
ashmaroli reviewed Mar 6, 2022
View reviewed changes
@ashmaroli
Copy link
Member
ashmaroli commented Mar 6, 2022 •
edited
Loading

If I don't use basename for name of the method in excerpt.rb. I get this error when trying to run jekyll..

I've left a suggestion in the review.

Plus, if you don't wish to continue working on this, feel free to close this PR.
I'll submit a fresh one from latest master.

@jjnilton
Copy link
Contributor Author
jjnilton commented Mar 6, 2022

Updated with the suggestion but it throws that error during the tests:

Error:
TestExcerptDrop#test_: an excerpt drop should be inspectable. :
NoMethodError: undefined method `basename' for <Jekyll::Excerpt id=/publish_test/2008/02/02/published#excerpt>:Jekyll::Excerpt
    /home/jj/Downloads/giveup/jekyll-1/lib/jekyll/drops/drop.rb:72:in `block in delegate_method_as'

There are other failures too, but it seems they appear even on a fresh clone from master without modifications.

@ashmaroli
Copy link
Member

Updated with the suggestion but it throws that error during the tests..

My bad! I did not notice that you added the definition to excerpt.rb instead of drops/excerpt/drop.rb.
I've made the necessary switch.

ashmaroli
ashmaroli approved these changes Mar 7, 2022
View reviewed changes
Copy link
Member
@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ashmaroli ashmaroli requested review from mattr- and parkr March 7, 2022 16:27
@ashmaroli
Copy link
Member

Thanks @jjnilton
@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 776748e into jekyll:master Apr 1, 2022
jekyllbot added a commit that referenced this pull request Apr 1, 2022
@jekyllbot
Update history to reflect merge of #8761 [ci skip]
3c46d84
github-actions bot pushed a commit that referenced this pull request Apr 1, 2022
@jekyllbot
Deploy docs from 776748e
e26dfbc 
JJ: Expose `basename` from `document.rb` as `name` to Liquid templates (#8761)

Merge pull request 8761
@marcwrobel marcwrobel mentioned this pull request Oct 21, 2022
Closed
@jekyll jekyll locked and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ashmaroli ashmaroli ashmaroli approved these changes

@mattr- mattr- Awaiting requested review from mattr-

@parkr parkr Awaiting requested review from parkr

Assignees
No one assigned
Labels
enhancement frozen-due-to-age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

{{ page.name }} doesn't return anything on posts
3 participants
@jjnilton @ashmaroli @jekyllbot
0