Skip to content

Conversation

@kamipo
Copy link
Member

@kamipo kamipo commented May 28, 2017

No description provided.

@kaspth
Copy link
Contributor

kaspth commented May 29, 2017

All these classes are nodoc'ed, why not keep the Git history intact and then move to private on Ruby 2.3+?

@kamipo kamipo force-pushed the dont_expose_methods_and_attrs_for_internal_usage branch from 00ff09c to 39f967e Compare May 29, 2017 07:30
@kamipo
Copy link
Member Author

kamipo commented May 29, 2017

I changed to keep the Git history intact as much as possible.

I think that correct visibility helps to understand the code (a reason like #26076).

The TODO comments in the previous implementation followed 21e5fd4. Probably the whole of the TODO comments will be removed and will be changed attrs visibility to private in the future.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't really private, so the change seems more misleading than explanatory.

Copy link
Contributor

@kaspth kaspth May 29, 2017

Choose a reason for hiding this comment

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

This seems worse than the originally proposed protected to me. History be damned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I'll change it back to originally proposed implementation.

@kaspth
Copy link
Contributor

kaspth commented May 29, 2017

Regardless of what I've said in the past, I think we should just leave these be and deal with them once we're Ruby 2.3+.

@matthewd your call 👍

@kamipo kamipo force-pushed the dont_expose_methods_and_attrs_for_internal_usage branch from 39f967e to c2c0807 Compare May 29, 2017 18:25
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't really private, so the change seems more misleading than explanatory.

Good point. Actually actual_source_reflection is called in ThroughReflection.
But ThroughReflection don't inherite AssociationReflection, so I moved actual_source_reflection from AssociationReflection to AbstractReflection.

@kamipo kamipo force-pushed the dont_expose_methods_and_attrs_for_internal_usage branch from c2c0807 to ceca77d Compare May 29, 2017 19:11
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually actual_source_reflection is called in ThroughReflection.
But ThroughReflection don't inherite AssociationReflection, so I moved actual_source_reflection from AssociationReflection to AbstractReflection.

@kamipo kamipo force-pushed the dont_expose_methods_and_attrs_for_internal_usage branch from ceca77d to 55f9b45 Compare May 30, 2017 00:33
@kamipo kamipo force-pushed the dont_expose_methods_and_attrs_for_internal_usage branch from 55f9b45 to 5734dcd Compare May 30, 2017 09:17
@matthewd matthewd merged commit f495e0f into rails:master May 30, 2017
@kamipo kamipo deleted the dont_expose_methods_and_attrs_for_internal_usage branch May 31, 2017 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants