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

rubocop: lib/jekyll/static_file.rb #5019

Merged
merged 3 commits into from Jun 18, 2016

Conversation

Projects
None yet
3 participants
@ayastreb
Contributor

ayastreb commented Jun 16, 2016

#4885

@@ -11,6 +8,7 @@ class StaticFile
# base - The String path to the <source>.
# dir - The String path between <source> and the file.
# name - The String filename of the file.
# rubocop: disable ParameterLists

This comment has been minimized.

@ayastreb

ayastreb Jun 16, 2016

Contributor

I'm not sure about this disabling...
It seems like second argument - base - is always taken from site.source variable:
https://github.com/jekyll/jekyll/blob/master/lib/jekyll/readers/static_file_reader.rb#L18
https://github.com/jekyll/jekyll/blob/master/lib/jekyll/collection.rb#L70

Do you think we should change the signature and usage of this class? Or just ignore it?

@ayastreb

ayastreb Jun 16, 2016

Contributor

I'm not sure about this disabling...
It seems like second argument - base - is always taken from site.source variable:
https://github.com/jekyll/jekyll/blob/master/lib/jekyll/readers/static_file_reader.rb#L18
https://github.com/jekyll/jekyll/blob/master/lib/jekyll/collection.rb#L70

Do you think we should change the signature and usage of this class? Or just ignore it?

This comment has been minimized.

@parkr

parkr Jun 16, 2016

Member

Due to SemVer, we are not able to change it until Jekyll 4.

@parkr

parkr Jun 16, 2016

Member

Due to SemVer, we are not able to change it until Jekyll 4.

@ayastreb ayastreb changed the title from rubocop: jekyll/lib/static_file.rb to rubocop: lib/jekyll/static_file.rb Jun 16, 2016

Show outdated Hide outdated test/test_site.rb
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 16, 2016

Member

This is looking great, thank you! Just one more comment 😄

Member

parkr commented Jun 16, 2016

This is looking great, thank you! Just one more comment 😄

@jekyllbot jekyllbot removed the needs-work label Jun 17, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 18, 2016

Member

Thank you!!

@jekyllbot: merge +dev

Member

parkr commented Jun 18, 2016

Thank you!!

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 5c03e1d into jekyll:master Jun 18, 2016

1 check passed

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

jekyllbot added a commit that referenced this pull request Jun 18, 2016

Crunch09 added a commit to Crunch09/jekyll that referenced this pull request Sep 6, 2016

Revert "Merge pull request #5019 from ayastreb/static_file"
This reverts commit 5c03e1d, reversing
changes made to b06cfb9.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment