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

Fixed stderr warnings #247

Merged
merged 1 commit into from
Mar 18, 2016
Merged

Fixed stderr warnings #247

merged 1 commit into from
Mar 18, 2016

Conversation

yakschuss
Copy link
Contributor

Deflected when using RUBYOPT=-w rake 2>&1 1>/dev/null | grep merit | sort --unique

@@ -47,7 +47,7 @@ def had_errors?
end

def target_object
target_obj = instance_variable_get(:"@#{controller_name.singularize}")
target_obj = instance_variable_defined? ? instance_variable_get(:"@#{controller_name.singularize}") : nil

Choose a reason for hiding this comment

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

Line is too long. [111/80]

Copy link
Member

Choose a reason for hiding this comment

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

@yakschuss we are not passing an argument to instance_variable_defined?! Suggested rewrite:

def target_object
  variable_name = :"@#{controller_name.singularize}"
  if instance_variable_defined?(variable_name)
    target_obj = instance_variable_get(variable_name)
    if target_obj.nil?
      str = '[merit] No object found, you might need a ' \
        "'@#{controller_name.singularize}' variable in " \
        "'#{controller_path}_controller' if no reputation is applied. " \
        'If you are using `model_name` option in the rule this is ok.'
      Rails.logger.warn str
    end
  end
  target_obj
end

Copy link
Member

Choose a reason for hiding this comment

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

Thar version can probably improved, or we might be able to extract another method off of it.

@tute tute mentioned this pull request Mar 14, 2016
"'#{controller_path}_controller' if no reputation is applied. " \
'If you are using `model_name` option in the rule this is ok.'
Rails.logger.warn str
end
Copy link
Member

Choose a reason for hiding this comment

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

Could we extract another method with this, so it reads something like:

warn_no_object_found if target_obj.nil?

@yakschuss
Copy link
Contributor Author

    def target_object
      variable_name = :"@#{controller_name.singularize}"
      target_obj = instance_variable_get(variable_name) if instance_variable_defined?(variable_name)
      target_obj.nil? ? warn_no_object_found : target_obj
    end

should work, as instance_variable_get would return nil if variable_name is not defined, but the warnings came back with that method.

maybe solution committed works for you

end
target_obj
target_obj.nil? ? warn_no_object_found : target_obj
Copy link
Member

Choose a reason for hiding this comment

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

In the case where target_obj.nil? this new code returns true, because that's what Rails.logger.warn. It should instead return nil, either explicit or implicit (no else branch in the if).

Copy link
Member

Choose a reason for hiding this comment

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

Looking better! Suggested rewrite, as per my comments:

def target_object
  variable_name = :"@#{controller_name.singularize}"
  if instance_variable_defined?(variable_name)
    if target_obj = instance_variable_get(variable_name)
      target_obj
    else
      warn_no_object_found
    end
  end
end

def warn_no_object_found
  str = '[merit] No object found, you might need a ' \
    "'@#{controller_name.singularize}' variable in " \
    "'#{controller_path}_controller' if no reputation is applied. " \
    'If you are using `model_name` option in the rule this is ok.'
  Rails.logger.warn str
  nil
end

@yakschuss
Copy link
Contributor Author

@tute - Thanks for the advice! I definitely learned something.

@tute
Copy link
Member

tute commented Mar 17, 2016

This is great! Can you please squash your commits together, add a meaningful commit message linking to the original issue, and push? Then I'll merge. Thank you! :)

tute added a commit that referenced this pull request Mar 18, 2016
@tute tute merged commit 9138d16 into merit-gem:master Mar 18, 2016
@tute
Copy link
Member

tute commented Mar 18, 2016

Thank you! :) 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants