Fix many warnings #4537

Merged
merged 1 commit into from Apr 1, 2016

Conversation

Projects
None yet
5 participants
@Crunch09
Member

Crunch09 commented Feb 17, 2016

Hi,
you can see all the warnings this PR removes in the commit message. This commit does not remove the following warnings:

  • lib/jekyll/url.rb:119:in 'escape_path': warning: URI.escape is obsolete
  • lib/jekyll/url.rb:133:in 'unescape_path': warning: URI.unescape is obsolete

which were mentioned in #4476 . I tried to replace the calls with CGI.escape / CGI.unescape but i couldn't get them to return the same results as the current implementation and i thought it would be too risky to replace it with a different behavior just to get rid of those warnings.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Feb 17, 2016

Contributor

Awesome, thanks for the submission, just a few comments!

Contributor

envygeeks commented Feb 17, 2016

Awesome, thanks for the submission, just a few comments!

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Feb 20, 2016

Member

@envygeeks @parkr Thanks for your review, i tried to address your concerns and updated the code.

Member

Crunch09 commented Feb 20, 2016

@envygeeks @parkr Thanks for your review, i tried to address your concerns and updated the code.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 18, 2016

Member

Hey @Crunch09! There are some problems with this PR, specifically around changes of behaviour. I will address them for you now.

Member

parkr commented Mar 18, 2016

Hey @Crunch09! There are some problems with this PR, specifically around changes of behaviour. I will address them for you now.

@@ -8,7 +8,9 @@ class Converter < Plugin
#
# Returns the String prefix.
def self.highlighter_prefix(highlighter_prefix = nil)
- @highlighter_prefix = highlighter_prefix if highlighter_prefix
+ if !defined?(@highlighter_prefix) || !highlighter_prefix.nil?

This comment has been minimized.

@parkr

parkr Mar 18, 2016

Member

This changes the behaviour slightly. Before: If highlighter_prefix is truthy, set its value to @highlighter_prefix. After: if highlighter_prefix is not nil or @highlighter_prefix isn't defined, then it sets @highlighter_prefix to highlighter_prefix.

Why add the !defined? check?

@parkr

parkr Mar 18, 2016

Member

This changes the behaviour slightly. Before: If highlighter_prefix is truthy, set its value to @highlighter_prefix. After: if highlighter_prefix is not nil or @highlighter_prefix isn't defined, then it sets @highlighter_prefix to highlighter_prefix.

Why add the !defined? check?

This comment has been minimized.

@Crunch09

Crunch09 Mar 20, 2016

Member

The description of the method says:

When an argument is specified, the prefix will be set. If no argument is specified, the current prefix will be returned.

That's why i check for nil (the default argument value) explicitly instead of just a truthy value. It seemed more accurate according to the comment to me.
I added the !defined? check to set the @higlighter_prefix in this specific case even if the argument is nil to remove the warning and initialize the instance variable with nil.

@Crunch09

Crunch09 Mar 20, 2016

Member

The description of the method says:

When an argument is specified, the prefix will be set. If no argument is specified, the current prefix will be returned.

That's why i check for nil (the default argument value) explicitly instead of just a truthy value. It seemed more accurate according to the comment to me.
I added the !defined? check to set the @higlighter_prefix in this specific case even if the argument is nil to remove the warning and initialize the instance variable with nil.

This comment has been minimized.

@parkr

parkr Mar 24, 2016

Member

@Crunch09 Absolutely! The change in behaviour is slight, but it is a change nevertheless that could cause problems. I think we'll go with it and revert in 3.2.1 if things go sour.

@parkr

parkr Mar 24, 2016

Member

@Crunch09 Absolutely! The change in behaviour is slight, but it is a change nevertheless that could cause problems. I think we'll go with it and revert in 3.2.1 if things go sour.

@@ -20,7 +22,9 @@ def self.highlighter_prefix(highlighter_prefix = nil)
#
# Returns the String suffix.
def self.highlighter_suffix(highlighter_suffix = nil)
- @highlighter_suffix = highlighter_suffix if highlighter_suffix
+ if !defined?(@highlighter_suffix) || !highlighter_suffix.nil?
+ @highlighter_suffix = highlighter_suffix

This comment has been minimized.

@parkr

parkr Mar 18, 2016

Member

Same concern as I had above with _prefix.

@parkr

parkr Mar 18, 2016

Member

Same concern as I had above with _prefix.

This comment has been minimized.

@Crunch09

Crunch09 Mar 20, 2016

Member

same reason as above :)

@Crunch09

Crunch09 Mar 20, 2016

Member

same reason as above :)

@@ -57,7 +57,7 @@ def make_accessible(hash = @config)
private
def highlighter
- return @highlighter if @highlighter
+ return @highlighter if @highlighter ||= nil

This comment has been minimized.

@parkr

parkr Mar 18, 2016

Member

This is redundant. Is there a way to get around this?

@parkr

parkr Mar 18, 2016

Member

This is redundant. Is there a way to get around this?

This comment has been minimized.

@Crunch09

Crunch09 Mar 20, 2016

Member

Maybe

return @highlighter if defined?(@highlighter) && @highlighter

but i'm not too happy with that either.

@Crunch09

Crunch09 Mar 20, 2016

Member

Maybe

return @highlighter if defined?(@highlighter) && @highlighter

but i'm not too happy with that either.

This comment has been minimized.

@parkr

parkr Mar 24, 2016

Member

Can we initialize @highlighter in initialize?

@parkr

parkr Mar 24, 2016

Member

Can we initialize @highlighter in initialize?

lib/jekyll/plugin.rb
- @safe = safe
- end
- @safe || false
+ @safe ||= !!safe

This comment has been minimized.

@parkr

parkr Mar 18, 2016

Member

This changes the behaviour. Before: If safe is truthy, set its value to @safe. After: set @safe to boolean equivalent of truthiness of safe only if @safe is not truthy.

This is a breaking change.

@parkr

parkr Mar 18, 2016

Member

This changes the behaviour. Before: If safe is truthy, set its value to @safe. After: set @safe to boolean equivalent of truthiness of safe only if @safe is not truthy.

This is a breaking change.

This comment has been minimized.

@mattr-

mattr- Mar 18, 2016

Member

Maybe this needs a comment? IIRC, I think I tried to do something similar awhile back.

@mattr-

mattr- Mar 18, 2016

Member

Maybe this needs a comment? IIRC, I think I tried to do something similar awhile back.

This comment has been minimized.

@Crunch09

Crunch09 Mar 20, 2016

Member

You're right i tried to change the code now similar to the _suffix / _prefix stuff because i think it is closest to the method description in this case as well.

@Crunch09

Crunch09 Mar 20, 2016

Member

You're right i tried to change the code now similar to the _suffix / _prefix stuff because i think it is closest to the method description in this case as well.

lib/jekyll/regenerator.rb
@@ -4,6 +4,7 @@ class Regenerator
def initialize(site)
@site = site
+ @disabled = nil

This comment has been minimized.

@parkr

parkr Mar 18, 2016

Member

Can we create a private attr_accessor? You may have to do this via:

attr_accessor :disabled
private :disabled, :disabled=

and change all references of @disabled to disabled and @disabled = to self.disabled =

@parkr

parkr Mar 18, 2016

Member

Can we create a private attr_accessor? You may have to do this via:

attr_accessor :disabled
private :disabled, :disabled=

and change all references of @disabled to disabled and @disabled = to self.disabled =

test/test_site.rb
@@ -519,7 +519,7 @@ def convert(*args)
contacts_html = @site.pages.find { |p| p.name == "contacts.html" }
@site.process
- source = @site.in_source_dir(contacts_html.path)
+ @site.in_source_dir(contacts_html.path)

This comment has been minimized.

@parkr

parkr Mar 18, 2016

Member

You can just remove this if the return value isn't used.

@parkr

parkr Mar 18, 2016

Member

You can just remove this if the return value isn't used.

@parkr parkr added this to the flexible milestone Mar 18, 2016

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Mar 20, 2016

Member

@parkr Thank you for the review! I tried to address all of your points and thought it would be best to create another commit. I'll squash them once everybody is okay with the changes.

Member

Crunch09 commented Mar 20, 2016

@parkr Thank you for the review! I tried to address all of your points and thought it would be best to create another commit. I'll squash them once everybody is okay with the changes.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 24, 2016

Member

@Crunch09 Thanks! One comment above in response to your kramdown_parser.rb reply and this should be good to go.

Member

parkr commented Mar 24, 2016

@Crunch09 Thanks! One comment above in response to your kramdown_parser.rb reply and this should be good to go.

Fix warnings
This removes the following warnings:

- /lib/jekyll/configuration.rb:151: warning: instance variable @default_config_file not initialized
- /lib/jekyll/converter.rb:12: warning: instance variable @highlighter_prefix not initialized
- /lib/jekyll/converter.rb:24: warning: instance variable @highlighter_suffix not initialized
- /lib/jekyll/converters/markdown.rb:9: warning: instance variable @setup not initialized
- /lib/jekyll/converters/markdown/kramdown_parser.rb:60: warning: instance variable @Highlighter not initialized
- /lib/jekyll/frontmatter_defaults.rb:97: warning: shadowing outer local variable - path
- /lib/jekyll/plugin.rb:66: warning: instance variable @safe not initialized
- /lib/jekyll/regenerator.rb:147: warning: instance variable @disabled not initialized
- /test/test_convertible.rb:40: warning: ambiguous first argument; put parentheses or a space even after `/' operator
- /test/test_filters.rb:154: warning: ambiguous first argument; put parentheses or a space even after `/' operator
- /test/test_new_command.rb:84: warning: ambiguous first argument; put parentheses or a space even after `/' operator
- /test/test_site.rb:234: warning: assigned but unused variable - site
- /test/test_site.rb:240: warning: assigned but unused variable - site
- /test/test_site.rb:522: warning: assigned but unused variable - source
- /test/test_tags.rb:153: warning: ambiguous first argument; put parentheses or a space even after `/' operator
- /test/test_tags.rb:425: warning: ambiguous first argument; put parentheses or a space even after `/' operator
- /test/test_tags.rb:449: warning: ambiguous first argument; put parentheses or a space even after `/' operator
- /test/test_tags.rb:496: warning: ambiguous first argument; put parentheses or a space even after `/' operator
- /test/test_tags.rb:496: warning: instance variable @Result not initialized
- /test/test_tags.rb:511: warning: ambiguous first argument; put parentheses or a space even after `/' operator
- /test/test_tags.rb:773: warning: ambiguous first argument; put parentheses or a space even after `/' operator
- /test/test_tags.rb:773: warning: instance variable @Result not initialized
- /test/test_tags.rb:788: warning: ambiguous first argument; put parentheses or a space even after `/' operator
- /test/test_url.rb:66: warning: shadowing outer local variable - doc
- /lib/jekyll/url.rb:119:in `escape_path': warning: URI.escape is obsolete
@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Mar 25, 2016

Member

@parkr Hey, i added the suggested initialization in kramdown_parser.rb and squashed the commits

Member

Crunch09 commented Mar 25, 2016

@parkr Hey, i added the suggested initialization in kramdown_parser.rb and squashed the commits

@parkr parkr changed the title from Fix warnings to Fix many warnings Apr 1, 2016

@parkr parkr modified the milestones: 3.2, flexible Apr 1, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 1, 2016

Member

@jekyllbot: merge +dev

Member

parkr commented Apr 1, 2016

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 6ee728e into jekyll:master Apr 1, 2016

1 check passed

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

jekyllbot added a commit that referenced this pull request Apr 1, 2016

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