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/utils.rb #5026

Merged
merged 1 commit into from Jun 22, 2016

Conversation

Projects
None yet
3 participants
@ayastreb
Contributor

ayastreb commented Jun 20, 2016

#4885

@@ -146,11 +134,13 @@ def parse_date(input, msg = "Input could not be parsed.")
# Determines whether a given file has
#
# Returns true if the YAML front matter is present.
# rubocop: disable PredicateName

This comment has been minimized.

@ayastreb

ayastreb Jun 20, 2016

Contributor

Rubocop suggest to rename has_yaml_header? to yaml_header? but can we do that?

@ayastreb

ayastreb Jun 20, 2016

Contributor

Rubocop suggest to rename has_yaml_header? to yaml_header? but can we do that?

This comment has been minimized.

@parkr

parkr Jun 21, 2016

Member

We cannot rename public methods, no. Unless it was marked private before, it cannot be renamed.

@parkr

parkr Jun 21, 2016

Member

We cannot rename public methods, no. Unless it was marked private before, it cannot be renamed.

end
private
def default_proc?(object)

This comment has been minimized.

@parkr

parkr Jun 21, 2016

Member

This method seems like it might be checking whether the object is a default proc. Which is a bit confusing. Perhaps we could just check if these are Hashes?

@parkr

parkr Jun 21, 2016

Member

This method seems like it might be checking whether the object is a default proc. Which is a bit confusing. Perhaps we could just check if these are Hashes?

This comment has been minimized.

@ayastreb

ayastreb Jun 22, 2016

Contributor

The original code was:

if target.respond_to?(:default_proc) && overwrite.respond_to?(:default_proc) && target.default_proc.nil?
  target.default_proc = overwrite.default_proc
end

I couldn't find the way to make if statement line shorter, so I created this default_proc? method.
Do you suggest to change the if condition to check if both target and overwrite are Hash?
Like this:

if target.is_a?(Hash) && overwrite.is_a?(Hash) && target.default_proc.nil?
  target.default_proc = overwrite.default_proc
end
@ayastreb

ayastreb Jun 22, 2016

Contributor

The original code was:

if target.respond_to?(:default_proc) && overwrite.respond_to?(:default_proc) && target.default_proc.nil?
  target.default_proc = overwrite.default_proc
end

I couldn't find the way to make if statement line shorter, so I created this default_proc? method.
Do you suggest to change the if condition to check if both target and overwrite are Hash?
Like this:

if target.is_a?(Hash) && overwrite.is_a?(Hash) && target.default_proc.nil?
  target.default_proc = overwrite.default_proc
end

This comment has been minimized.

@parkr

parkr Jun 22, 2016

Member

Do you suggest to change the if condition to check if both target and overwrite are Hash?

That is one solution, yes. I don't think Drops support default_proc, so there is no need to try to merge a default proc for anything but a Hash.

@parkr

parkr Jun 22, 2016

Member

Do you suggest to change the if condition to check if both target and overwrite are Hash?

That is one solution, yes. I don't think Drops support default_proc, so there is no need to try to merge a default proc for anything but a Hash.

elsif mergable?(old_val) && mergable?(new_val)
deep_merge_hashes(old_val, new_val)
else
new_val

This comment has been minimized.

@parkr

parkr Jun 21, 2016

Member

We could probably get away with doing a new_val.frozen? ? new_val.dup : new_val here, or using a method like unfreeze(new_val) to do the same for both this and old_val on line 294.

@parkr

parkr Jun 21, 2016

Member

We could probably get away with doing a new_val.frozen? ? new_val.dup : new_val here, or using a method like unfreeze(new_val) to do the same for both this and old_val on line 294.

This comment has been minimized.

@ayastreb

ayastreb Jun 22, 2016

Contributor

I just tried and a lot of tests fail with:
TypeError: can't dup FalseClass
looks like we need to check if the value is duplicable?.
But it was not duplicating values in the original code here, do we need to do it?

@ayastreb

ayastreb Jun 22, 2016

Contributor

I just tried and a lot of tests fail with:
TypeError: can't dup FalseClass
looks like we need to check if the value is duplicable?.
But it was not duplicating values in the original code here, do we need to do it?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 21, 2016

Member

Thanks, @ayastreb! Just two thoughts above.

Member

parkr commented Jun 21, 2016

Thanks, @ayastreb! Just two thoughts above.

@parkr parkr referenced this pull request Jun 21, 2016

Closed

Make jekyll/jekyll code compliant with rubocop rules #4885

114 of 115 tasks complete
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 22, 2016

Member

@jekyllbot: merge +dev

Member

parkr commented Jun 22, 2016

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 6358a8f into jekyll:master Jun 22, 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 22, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 22, 2016

Member

Thanks! I will take care of my nit-picks on master. 😄

Member

parkr commented Jun 22, 2016

Thanks! I will take care of my nit-picks on master. 😄

@ayastreb

This comment has been minimized.

Show comment
Hide comment
@ayastreb

ayastreb Jun 22, 2016

Contributor

Great! Thanks!
On Jun 22, 2016 20:06, "Parker Moore" notifications@github.com wrote:

Thanks! I will take care of my nit-picks on master. 😄


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5026 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ACpiQ0fELfO_UbACA0ECxjs_i0Q2Qjk5ks5qOWuugaJpZM4I5rXR
.

Contributor

ayastreb commented Jun 22, 2016

Great! Thanks!
On Jun 22, 2016 20:06, "Parker Moore" notifications@github.com wrote:

Thanks! I will take care of my nit-picks on master. 😄


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5026 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ACpiQ0fELfO_UbACA0ECxjs_i0Q2Qjk5ks5qOWuugaJpZM4I5rXR
.

parkr added a commit that referenced this pull request Jun 22, 2016

@ayastreb ayastreb deleted the ayastreb:utils branch Jun 22, 2016

parkr added a commit that referenced this pull request Jun 29, 2016

Merge branch 'master' into remove-jruby-and-ruby-head
* master: (41 commits)
  Fix rubocop offenses on master.
  script/fmt: print Rubocop version
  Update history to reflect merge of #5030 [ci skip]
  rubocop: separate deprecator error messages
  Update history to reflect merge of #5031 [ci skip]
  Update history to reflect merge of #5032 [ci skip]
  rubocop: fix code style
  rubocop: fix code style
  rubocop: fix code style
  Update history to reflect merge of #5024 [ci skip]
  Update history to reflect merge of #5025 [ci skip]
  utils: check that the object is a hash when merging default_proc
  Update history to reflect merge of #5026 [ci skip]
  rubocop: refactor modified? method
  Add a benchmark for capture vs. assign in Liquid. [ci skip]
  Update history to reflect merge of #5027 [ci skip]
  Add generator-jekyllized to third party plugins
  rubocop: fix code style
  rubocop: fix code style
  rubocop: fix code style
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment