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

👎 on Rails/HelperInstanceVariable #28

Closed
searls opened this issue Feb 10, 2024 · 4 comments
Closed

👎 on Rails/HelperInstanceVariable #28

searls opened this issue Feb 10, 2024 · 4 comments

Comments

@searls
Copy link
Contributor

searls commented Feb 10, 2024

I'm using standard-rails in earnest for the first time so I suspect there will be a few issues like this soon.

module HeadHelper
  def page_title
    if post? && @post.title.present?
      "#{@post.title} | #{site_name}"
    else
      site_name
    end
  end

  def post?
    instance_variable_get(:@post).present?
  end
end

This fails with Do not use instance variables in helpers.standard(Rails/HelperInstanceVariable), even though there's a check right there that the ivar is available

@mjankowski
Copy link
Contributor

This one is enabled by default in rubocop-rails, and I think it generally makes sense to leave enabled and require per-usage disable if desired.

In the issue example (what I'm assuming is setting an html title element on a page in a generic way that will work across the whole app?), I'd probably look for a way to either:

  • Pass in page_title(value) and then use value || site_name type logic in method;
  • Use content_for(:page_title) from the views and then check there in the helper method;
  • Use one of the i18n/interpolation-supporting title gems and use their documented approach to pass in the i-var

@dogweather
Copy link
Contributor

Interesting. Reading the cop docs, the intent seems to be,

Relying on instance variables makes it difficult to reuse helper methods.

But @searls your design accounts for this.

BTW, I like your design of having the logic for all the HTML titles in one place. Because, these are usu. part of a comprehensive HTML metadata strategy. So it'd be nice to see them all in one place.

@searls
Copy link
Contributor Author

searls commented Jul 2, 2024

Yeah I think I'd be cool with a PR disabling this

@searls
Copy link
Contributor Author

searls commented Aug 26, 2024

I ended up turning this off b/c it was a show-stopper to getting my new app standard-rails compliant :yolo: 2733934

@searls searls closed this as completed Aug 26, 2024
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

No branches or pull requests

3 participants