Skip to content

prefix TemplateAssertions ivars (#7459) #7797

Merged
merged 1 commit into from Oct 2, 2012

3 participants

@senny
Ruby on Rails member
senny commented Sep 30, 2012

I changed the variables @layouts, @partials and @templates to be prefixed with an underscore. The rails tests pass but of course this could make an impact on test cases, which use these internal variables (I don't hope these kind of tests exist though ;)

@senny
Ruby on Rails member
senny commented Sep 30, 2012

@steveklabnik, @rafaelfranca please have a look. I noticed that there are more internal variables. Should they be prefixed too? I don't think we should try to minimize naming conflicts as much as possible. The current list is:

      INTERNAL_IVARS = [
        :@__name__,
        :@__io__,
        :@_assertion_wrapped,
        :@_assertions,
        :@_result,
        :@_routes,
        :@controller,
        :@_layouts,
        :@locals,
        :@method_name,
        :@output_buffer,
        :@_partials,
        :@passed,
        :@rendered,
        :@request,
        :@routes,
        :@tagged_logger,
        :@_templates,
        :@options,
        :@test_passed,
        :@view,
        :@view_context_class
      ]
@senny
Ruby on Rails member
senny commented Oct 1, 2012

@rafaelfranca rebased and added a CHANGELOG entry for this one too.

@rafaelfranca
Ruby on Rails member

@senny it seems fine. I don't know about all these variables but one in special took me attention. We have both @_routes and @route in that array. Could you investigate?

I'm merging this now. Thanks.

@rafaelfranca rafaelfranca merged commit f934aec into rails:master Oct 2, 2012
@senny
Ruby on Rails member
senny commented Oct 2, 2012

@rafaelfranca thanks, I'll have a look at the routing variables.

@senny
Ruby on Rails member
senny commented Oct 4, 2012

@rafaelfranca I did some research on @routes and @_routes. Currently they are both needed and serve the following purpose:

  • ´@routes´ is the RouteSet instance and is only used for testing
  • ´@_routes´ is from UrlFor, which is mixed into the TestCase. This variable is also used in production

I could submit a PR to rename @routes to @_route_set. What do you think?

@senny
Ruby on Rails member
senny commented Oct 4, 2012

I just noticed that @routes is accessed from everywhere in the tests. I think we should not touch this code.

@rafaelfranca
Ruby on Rails member

hmm. So I think we can't do anything :(

@steveklabnik
Ruby on Rails member

At least for now... yay encapsulation! :(

@rafaelfranca rafaelfranca added a commit that referenced this pull request Oct 31, 2012
@rafaelfranca rafaelfranca Revert "Merge pull request #7797 from senny/7459_prefix_tempalte_asse…
…rtion_variables"

This reverts commit 2bad605.

Conflicts:
	actionpack/CHANGELOG.md

Reason: This added a regression related with shoulda-matchers, since it
is expecting the instance variable @layouts

See https://github.com/thoughtbot/shoulda-matchers/blob/9e1188eea68c47d9a56ce6280e45027da6187ab1/lib/shoulda/matchers/action_controller/render_with_layout_matcher.rb#L74

This will introduce back #7459 but this stable release will be backward compatible.
Related with #8068.
6b7cd20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.