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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt StaticFile for collections, config defaults #3823

Merged
merged 1 commit into from Jul 1, 2015

Conversation

Projects
None yet
5 participants
@mbland
Contributor

mbland commented Jun 25, 2015

This enables files such as images and PDFs to show up in the same relative output directory as other HTML and Markdown documents in the same collection.

It also enables static files to be hidden using defaults from _config.yml in the same way that other documents in the same collection and directories may be hidden using published: false.

Extracted from 18F/hub#332.

cc: @afeld @gboone @jeremiak @dhcole

@envygeeks

This comment has been minimized.

Contributor

envygeeks commented Jun 25, 2015

I like it 👍

@mbland mbland force-pushed the 18F:adapt-static-file branch from 0743afd to 3f90a2a Jun 25, 2015

mbland added a commit to 18F/jekyll that referenced this pull request Jun 25, 2015

@mbland

This comment has been minimized.

Contributor

mbland commented Jun 25, 2015

@envygeeks Thanks for the rapid feedback. All comments incorporated!

@envygeeks

This comment has been minimized.

Contributor

envygeeks commented Jun 25, 2015

Do you mind rebasing the two commits into a single commit and forcing it up again so that other reviewers can see both the changes and everything in on swift review? Thanks!

@mbland mbland force-pushed the 18F:adapt-static-file branch from 3f90a2a to c66a874 Jun 25, 2015

@mbland

This comment has been minimized.

Contributor

mbland commented Jun 25, 2015

No problem! Done.

@dhcole

This comment has been minimized.

Contributor

dhcole commented Jun 25, 2015

Nice work @mbland. Would be great to see this land in Jekyll core.

title: '',
},
}).to_s
@url = @url.gsub /\/$/, ''

This comment has been minimized.

@parkr

parkr Jun 29, 2015

Member

this is effectively @url.gsub! – why not use the bang method here?

This comment has been minimized.

@mbland

mbland Jun 29, 2015

Contributor

Done.

I must've been thinking about how gsub! will return nil if nothing was changed, and how that would not work as a return value. But it's not a return value here.

# the collection's URL template into account. The default URL template can
# be overriden in the collection's configuration in _config.yml.
def url
return @url unless @url.nil?

This comment has been minimized.

@parkr

parkr Jun 29, 2015

Member

haven't seen this in a while – why not something like

@url ||= if @collection.nil?
  relative_path
else
  Jekyll::URL.new({
    # etc
  })
end.to_s.gsub!(/\/$/, '')

This comment has been minimized.

@mbland

mbland Jun 29, 2015

Contributor

Wow, had no idea that was possible... Done, though I did have to remove the ! from gsub!, as that will return nil if no substitutions are performed.

else
@url = ::Jekyll::URL.new({
template: @collection.url_template,
placeholders: {

This comment has been minimized.

@parkr

parkr Jun 29, 2015

Member

would you please create a separate method which creates the placeholders hash?

This comment has been minimized.

@parkr

parkr Jun 29, 2015

Member

can we leverage any of #to_liquid?

This comment has been minimized.

@mbland

mbland Jun 29, 2015

Contributor

placeholders method: Done.

Regarding #to_liquid, I don't see any overlap between the values.

@parkr

This comment has been minimized.

Member

parkr commented Jun 29, 2015

Looking pretty good! Thanks, @mbland.

Looks like you're coming from a Python background, based on all those self.'s. In Ruby, we only use them when setting an attribute on a class instance, such as self.url = "some new url". When we access the attribute, however, we do not include the self. If you would please remove each instance of self. whenever accessing instance variables or methods, I would be most appreciative!

mbland added a commit to 18F/jekyll that referenced this pull request Jun 29, 2015

@mbland mbland force-pushed the 18F:adapt-static-file branch from fa282bb to 80ae0da Jun 29, 2015

mbland added a commit to 18F/jekyll that referenced this pull request Jun 29, 2015

@mbland

This comment has been minimized.

Contributor

mbland commented Jun 29, 2015

Thanks, @parkr! Took out all the self.s, though it's actually @afeld's fault I started using them in Ruby. :-P

@envygeeks

This comment has been minimized.

Contributor

envygeeks commented Jun 29, 2015

Can you please rebase all this into a single commit so that it can be re-reviewed in one swift shot.

Thanks.

Adapt StaticFile for collections, config defaults
This enables files such as images and PDFs to show up in the same relative
output directory as other HTML and Markdown documents in the same collection.

It also enables static files to be hidden using defaults from _config.yml in
the same way that other documents in the same collection and directories may
be hidden using `published: false`.

@mbland mbland force-pushed the 18F:adapt-static-file branch from 80ae0da to 250b6eb Jun 29, 2015

@mbland

This comment has been minimized.

Contributor

mbland commented Jun 29, 2015

Yep! Done.

@parkr

This comment has been minimized.

Member

parkr commented Jun 30, 2015

Cool! :shipit:

@mbland

This comment has been minimized.

Contributor

mbland commented Jul 1, 2015

Hey gang, not to nag, but is there further review pending, or is this ready to merge?

@envygeeks

This comment has been minimized.

Contributor

envygeeks commented Jul 1, 2015

This is ready! Sorry I didn't notice @parkr said to :shipit:

envygeeks added a commit that referenced this pull request Jul 1, 2015

Merge pull request #3823 from 18F/adapt-static-file
Adapt StaticFile for collections, config defaults

@envygeeks envygeeks merged commit 3a49770 into jekyll:master Jul 1, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

envygeeks added a commit that referenced this pull request Jul 1, 2015

@mbland

This comment has been minimized.

Contributor

mbland commented Jul 1, 2015

Sweeet, thanks, @envygeeks!

@mbland mbland deleted the 18F:adapt-static-file branch Jul 7, 2015

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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