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.

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

Already on GitHub? Sign in to your account

Add `static_file.name` and `.basename` Liquid attributes #5264

Merged
merged 4 commits into from Sep 29, 2016

Conversation

Projects
None yet
5 participants
@DirtyF
Member

DirtyF commented Aug 20, 2016

Disclaimer: These are my first ruby lines 馃槄

A proposal to fix #5263

Show outdated Hide outdated lib/jekyll/static_file.rb
@@ -97,6 +97,7 @@ def write(dest)
def to_liquid
{
"filename" => File.basename(name, extname),

This comment has been minimized.

@DirtyF

DirtyF Aug 20, 2016

Member

Or simply use name in order to simple call file.name in liquid?

@DirtyF

DirtyF Aug 20, 2016

Member

Or simply use name in order to simple call file.name in liquid?

@@ -1,6 +1,6 @@
module Jekyll
class StaticFile
attr_reader :relative_path, :extname
attr_reader :relative_path, :extname, :name

This comment has been minimized.

@parkr

parkr Aug 22, 2016

Member

This is a short way of defining:

def name
  @name
end

Therefore, the best thing to do here is to set @name in the initialize method using the code you have on line 100 and then, when you are constructing to_liquid below, you can just call the name method instead of re-calculating its value. Does that make sense?

@parkr

parkr Aug 22, 2016

Member

This is a short way of defining:

def name
  @name
end

Therefore, the best thing to do here is to set @name in the initialize method using the code you have on line 100 and then, when you are constructing to_liquid below, you can just call the name method instead of re-calculating its value. Does that make sense?

This comment has been minimized.

@DirtyF

DirtyF Aug 22, 2016

Member

@parkr yes, but then it breaks the tests as name used to be the whole name of the file including the extension. 馃槙

@DirtyF

DirtyF Aug 22, 2016

Member

@parkr yes, but then it breaks the tests as name used to be the whole name of the file including the extension. 馃槙

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 22, 2016

Member

Well done, @DirtyF! This is great. I left a comment above which should help you get this ready for a merge. 馃槃 And well done adding a test! I would love to also see a test for the name attr_reader you added, too, just to make sure it keeps working as we expect.

馃帀

Member

parkr commented Aug 22, 2016

Well done, @DirtyF! This is great. I left a comment above which should help you get this ready for a merge. 馃槃 And well done adding a test! I would love to also see a test for the name attr_reader you added, too, just to make sure it keeps working as we expect.

馃帀

@parkr parkr added this to the 3.2.2 milestone Aug 22, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 23, 2016

Contributor

I'm not against this but the LGTM is being witheld since this is a breaking change and there is no backwards compatibility being kept until 4.0, can somebody explain to me what the ramifications of this change are?

Contributor

envygeeks commented Aug 23, 2016

I'm not against this but the LGTM is being witheld since this is a breaking change and there is no backwards compatibility being kept until 4.0, can somebody explain to me what the ramifications of this change are?

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Aug 23, 2016

Member

@envygeeks I didn't intend a breaking change, only wanted to add an extra Liquid shortcut to access the name of a static file without having to use liquid filters to remove the extra parts.

Member

DirtyF commented Aug 23, 2016

@envygeeks I didn't intend a breaking change, only wanted to add an extra Liquid shortcut to access the name of a static file without having to use liquid filters to remove the extra parts.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 23, 2016

Contributor

@DirtyF Well, the question really is, can I still access it the old way if I want to?

Contributor

envygeeks commented Aug 23, 2016

@DirtyF Well, the question really is, can I still access it the old way if I want to?

@parkr

This comment has been minimized.

Show comment
Hide comment
Member

parkr commented Aug 24, 2016

@parkr parkr assigned oe Aug 24, 2016

Show outdated Hide outdated lib/jekyll/static_file.rb
@@ -97,6 +97,7 @@ def write(dest)
def to_liquid
{
"name" => File.basename(name, extname),

This comment has been minimized.

@parkr

parkr Aug 24, 2016

Member

I'm fine with adding this to Liquid. When you do static_file.name in Ruby, it expects something else, though?

@parkr

parkr Aug 24, 2016

Member

I'm fine with adding this to Liquid. When you do static_file.name in Ruby, it expects something else, though?

This comment has been minimized.

@DirtyF

DirtyF Aug 26, 2016

Member

From what I understand, in the initialize function, name is the complete name of the file including the extension.

I just modified the output in Liquid, to be able to get only the name without the extension, already available as extname. I did not want to modify the current behavior, as it would break all current tests.

As a total beginner, I have no clue how to properly achieve this. I just thought that we got to keep name as it is, and just add filename in the to_liquid function to be able to use both.

@DirtyF

DirtyF Aug 26, 2016

Member

From what I understand, in the initialize function, name is the complete name of the file including the extension.

I just modified the output in Liquid, to be able to get only the name without the extension, already available as extname. I did not want to modify the current behavior, as it would break all current tests.

As a total beginner, I have no clue how to properly achieve this. I just thought that we got to keep name as it is, and just add filename in the to_liquid function to be able to use both.

@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Aug 26, 2016

Member

@envygeeks what do you mean 'the old way'? all i see in this change is additions, nothing breaking

Member

oe commented Aug 26, 2016

@envygeeks what do you mean 'the old way'? all i see in this change is additions, nothing breaking

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 26, 2016

Member

Interesting, this change LGTM. I think the main issue was the possibility that StaticFile#name (in Ruby) would be changed if he implemented my suggestion.

@DirtyF, would you please leave a comment next to your addition to to_liquid explaining, essentially, that the value is to match the name value in Page?

Member

parkr commented Aug 26, 2016

Interesting, this change LGTM. I think the main issue was the possibility that StaticFile#name (in Ruby) would be changed if he implemented my suggestion.

@DirtyF, would you please leave a comment next to your addition to to_liquid explaining, essentially, that the value is to match the name value in Page?

@jekyllbot jekyllbot removed the needs-work label Aug 27, 2016

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Aug 27, 2016

Member

@parkr I don't get what you mean 馃槙

  • {{ page.name }} returns about.md
  • {{ file.name }} would return static_file not static_file.txt (cf. use case in #5263)

So it doesn't match Page, and that is a concern. Open to suggestions here (naming things is hard). I can revert 326332a

I completed the documentation with explicit examples for liquid metadata available for static files.

Member

DirtyF commented Aug 27, 2016

@parkr I don't get what you mean 馃槙

  • {{ page.name }} returns about.md
  • {{ file.name }} would return static_file not static_file.txt (cf. use case in #5263)

So it doesn't match Page, and that is a concern. Open to suggestions here (naming things is hard). I can revert 326332a

I completed the documentation with explicit examples for liquid metadata available for static files.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 27, 2016

@DirtyF I'd suggest naming it basename (and providing the name property as well).

Other, unrelated note: would we see an improvement using Jekyll Drops for these things?

ghost commented Aug 27, 2016

@DirtyF I'd suggest naming it basename (and providing the name property as well).

Other, unrelated note: would we see an improvement using Jekyll Drops for these things?

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Aug 27, 2016

Member

@spudowiar you're right basename is far less confusing and more explicit.

Documentation updated to reflect the changes.

docs-static-files

Member

DirtyF commented Aug 27, 2016

@spudowiar you're right basename is far less confusing and more explicit.

Documentation updated to reflect the changes.

docs-static-files

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 31, 2016

Contributor

LGTM. /cc @jekyll/core

Contributor

envygeeks commented Aug 31, 2016

LGTM. /cc @jekyll/core

@parkr parkr modified the milestones: 3.2.2, 3.3 Sep 22, 2016

@parkr

parkr approved these changes Sep 23, 2016

@DirtyF Remind me: is this consistent with page.name for Pages and Documents?

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Sep 23, 2016

Member

@parkr yes it is.

Member

DirtyF commented Sep 23, 2016

@parkr yes it is.

@parkr parkr changed the title from Get static filename to Add `static_file.name` and `.basename` Liquid attributes Sep 29, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 29, 2016

Member

This should eventually become a Drop. For now, LGTM.

@jekyllbot: merge +minor

Member

parkr commented Sep 29, 2016

This should eventually become a Drop. For now, LGTM.

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit ef3f9d0 into jekyll:master Sep 29, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
jekyll/lgtm Approved by @envygeeks. Requires 1 more LGTM.
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Sep 29, 2016

@DirtyF DirtyF deleted the DirtyF:static-file-name branch Oct 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment