Simplify _prefixes #15026

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
3 participants
@apotonick
Contributor

apotonick commented May 8, 2014

Summary

This simplifies the logic to compile the _prefixes array used for view inheritance. The change allows overriding ::_local_prefixes which didn't work before.

Implementation

It is now implemented with a proper recursion travelling up the inheritance chain instead of doing all the work locally in the concrete controller. Controllers in the chain can now define their local _prefixes by overriding _local_prefixes (we can rename that). This didn't work before.

We use this extensively in Cells for the new Trailblazer file layout. I thought it might be helpful to push that back to the core.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim May 8, 2014

Contributor

If the goal is to make _local_prefixes overridable, it needs to be properly documented, tested and probably lose the underscore. Also, I would rather have such in the instance (maybe as prefixes) rather than in the class.

Contributor

josevalim commented May 8, 2014

If the goal is to make _local_prefixes overridable, it needs to be properly documented, tested and probably lose the underscore. Also, I would rather have such in the instance (maybe as prefixes) rather than in the class.

simplify AC:ViewPaths::_prefixes. by making it recursively traversing…
… up the inheritance chain, classes can override local prefixes.
@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick May 8, 2014

Contributor

Totally agree about the proper documentation and testing and I will add that once you give me the OK 😁

Overriding on the instance level is still possible by overriding #_prefixes. As this is pretty uncommon, overriding ::_local_prefixes would make sense for most parties.

I don't know what's the deal with those underscored methods, we can remove them, I don't see what they stand for.

Thanks for your prompt relpy, @josevalim!

Contributor

apotonick commented May 8, 2014

Totally agree about the proper documentation and testing and I will add that once you give me the OK 😁

Overriding on the instance level is still possible by overriding #_prefixes. As this is pretty uncommon, overriding ::_local_prefixes would make sense for most parties.

I don't know what's the deal with those underscored methods, we can remove them, I don't see what they stand for.

Thanks for your prompt relpy, @josevalim!

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim May 8, 2014

Contributor

Underscored methods are private. They are not supposed to be removed if they are meant to be private.

Contributor

josevalim commented May 8, 2014

Underscored methods are private. They are not supposed to be removed if they are meant to be private.

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick May 8, 2014

Contributor

The problem is that those methods should be public but might interfere with actual controller actions. If ::_local_prefixes remained "private" what would be the problem with other controllers overriding it? Would that be an interface violation or how do you mean "not supposed to be removed"?

I personally find it OK to leave them underscored, just to avoid the hassle with name clashes.

Other than that my only concern is that I ruthlessly removed parent_prefixes without deprecating it. That should be alright?!

Contributor

apotonick commented May 8, 2014

The problem is that those methods should be public but might interfere with actual controller actions. If ::_local_prefixes remained "private" what would be the problem with other controllers overriding it? Would that be an interface violation or how do you mean "not supposed to be removed"?

I personally find it OK to leave them underscored, just to avoid the hassle with name clashes.

Other than that my only concern is that I ruthlessly removed parent_prefixes without deprecating it. That should be alright?!

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick May 8, 2014

Contributor

Here's what I wanna do.

  1. Rename ::_local_prefixes to ::local_prefixes to signalise it is meant to be overridden.
  2. Document the latter.
  3. Test the latter.
  4. Document removal of parent_prefixes.
Contributor

apotonick commented May 8, 2014

Here's what I wanna do.

  1. Rename ::_local_prefixes to ::local_prefixes to signalise it is meant to be overridden.
  2. Document the latter.
  3. Test the latter.
  4. Document removal of parent_prefixes.
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 9, 2014

Member

@apotonick seems a good plan.

Also I think if parent_prefix still can be implemented is better to deprecate first.

Member

rafaelfranca commented May 9, 2014

@apotonick seems a good plan.

Also I think if parent_prefix still can be implemented is better to deprecate first.

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick May 9, 2014

Contributor

@rafaelfranca Do you have an example where a method is marked as deprecated if it is defined (in Rails)? I will then inform the user about the deprecation of ::parent_prefixes as I am a nice person.

Contributor

apotonick commented May 9, 2014

@rafaelfranca Do you have an example where a method is marked as deprecated if it is defined (in Rails)? I will then inform the user about the deprecation of ::parent_prefixes as I am a nice person.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 9, 2014

Member

hmmm. I don't think we have this. I thought we want to deprecate the usage not the definition. So I guess the only way if checking when calling _prefixes on the instance. If there is a parent_prefixes defined we show the warning.

Member

rafaelfranca commented May 9, 2014

hmmm. I don't think we have this. I thought we want to deprecate the usage not the definition. So I guess the only way if checking when calling _prefixes on the instance. If there is a parent_prefixes defined we show the warning.

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick May 9, 2014

Contributor

Yeah, that's what I meant as well - I just thought there's an automation in Rails for that 😉

Contributor

apotonick commented May 9, 2014

Yeah, that's what I meant as well - I just thought there's an automation in Rails for that 😉

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 9, 2014

Member

There are some helpers but I prefer to call ActiveSupport::Deprecation.warn directly.

Member

rafaelfranca commented May 9, 2014

There are some helpers but I prefer to call ActiveSupport::Deprecation.warn directly.

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick May 10, 2014

Contributor

This guy is ready to be merged, @rafaelfranca and @josevalim.

Contributor

apotonick commented May 10, 2014

This guy is ready to be merged, @rafaelfranca and @josevalim.

+ [controller_path]
+ end
+
+ def handle_deprecated_parent_prefixes # TODO: remove in 4.3/5.0.

This comment has been minimized.

@rafaelfranca

rafaelfranca May 10, 2014

Member

This method should be private

@rafaelfranca

rafaelfranca May 10, 2014

Member

This method should be private

end
+

This comment has been minimized.

@rafaelfranca

rafaelfranca May 10, 2014

Member

✂️

+ include ActionView::Rendering
+ append_view_path File.expand_path(File.join(File.dirname(__FILE__), "views"))
+
+

This comment has been minimized.

@rafaelfranca

rafaelfranca May 10, 2014

Member

✂️

+ end
+ end
+
+

This comment has been minimized.

@rafaelfranca

rafaelfranca May 10, 2014

Member

✂️

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 10, 2014

Member

We still need tests overriding local_prefixes. Also _prefix is a private method still (because of the _) so I'd not recommend to override it on the CHANGELOG and documentation.

Member

rafaelfranca commented May 10, 2014

We still need tests overriding local_prefixes. Also _prefix is a private method still (because of the _) so I'd not recommend to override it on the CHANGELOG and documentation.

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick May 11, 2014

Contributor

I added tests for overriding ::local_prefixes, made it private and removed references to _parent_prefixes in the docs. 🎆

Contributor

apotonick commented May 11, 2014

I added tests for overriding ::local_prefixes, made it private and removed references to _parent_prefixes in the docs. 🎆

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick May 12, 2014

Contributor

Hi @rafaelfranca - this is ready to be merged, rebased and squashed in https://github.com/apotonick/rails/tree/simplify-prefixes. Thanks ❤️

Contributor

apotonick commented May 12, 2014

Hi @rafaelfranca - this is ready to be merged, rebased and squashed in https://github.com/apotonick/rails/tree/simplify-prefixes. Thanks ❤️

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick May 14, 2014

Contributor

Do you want me to push --force rebased/squashed branches to the original PR branch next time, so the commit meta data remains untouched (e.g. pushing simplify-prefixes (the new, squashed branch) to simpler-prefixes (this branch of this PR)?

Contributor

apotonick commented May 14, 2014

Do you want me to push --force rebased/squashed branches to the original PR branch next time, so the commit meta data remains untouched (e.g. pushing simplify-prefixes (the new, squashed branch) to simpler-prefixes (this branch of this PR)?

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick May 14, 2014

Contributor

Oh, and thanks!

Contributor

apotonick commented May 14, 2014

Oh, and thanks!

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 14, 2014

Member

Yes. For us it is better.

Member

rafaelfranca commented May 14, 2014

Yes. For us it is better.

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick May 14, 2014

Contributor

For me, too!

And again, thanks for this prompt and uncomplicated merge, I really really appreciate the "open-ness" in the core towards architectural changes. ❤️

Contributor

apotonick commented May 14, 2014

For me, too!

And again, thanks for this prompt and uncomplicated merge, I really really appreciate the "open-ness" in the core towards architectural changes. ❤️

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