Skip to content
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/collection.rb #5022

Merged
merged 2 commits into from Jul 16, 2016
Merged

Conversation

ayastreb
Copy link
Contributor

@ayastreb
Copy link
Contributor Author

@parkr any thoughts about this PR? 😄

else
Jekyll.logger.debug "Skipped From Publishing:", doc.relative_path
end
read_docs(full_path)
Copy link
Member

Choose a reason for hiding this comment

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

In this case, you're just reading one document. Thus, this should be singular: read_document(full_path).

@parkr
Copy link
Member

parkr commented Jul 15, 2016

Two naming nits. Otherwise, LGTM.

@parkr
Copy link
Member

parkr commented Jul 15, 2016

@jekyll/core, could one of you take a look at this and give it a LGTM?


private
def read_docs(full_path)
doc = Jekyll::Document.new(full_path, { :site => site, :collection => self })
Copy link
Contributor

Choose a reason for hiding this comment

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

doc = Document.new(full_path, {
  :site => site, :collection => self
})

doc.read
if site.publisher.publish?(doc) || !write?

Copy link
Member

Choose a reason for hiding this comment

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

You don't like the 1-line solution there? I think it reads nicely. We could even remove the curly braces:

doc = Document.new(full_path, :site => site, :collection => self)

Copy link
Contributor

Choose a reason for hiding this comment

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

I do like the version you've given @parkr.

@envygeeks
Copy link
Contributor

Just a few comments! Thanks.

@parkr
Copy link
Member

parkr commented Jul 16, 2016

LGTM. @envygeeks?

@envygeeks
Copy link
Contributor

LGTM.

@parkr
Copy link
Member

parkr commented Jul 16, 2016

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit cfeb54d into jekyll:master Jul 16, 2016
jekyllbot added a commit that referenced this pull request Jul 16, 2016
stevecheckoway pushed a commit to stevecheckoway/jekyll that referenced this pull request Jul 24, 2016
@jekyll jekyll locked and limited conversation to collaborators Jul 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants