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

v2: Rename jQuery View hook methods #3703

Closed
wants to merge 1 commit into from

Conversation

jridgewell
Copy link
Collaborator

DO NOT MERGE, BREAKING CHANGE This is for the next major, v2.

To distinguish these methods from “private” methods (i.e. #_ensureElement).

To distinguish these methods from “private” functions (i.e.
`#_ensureElement`).
@jridgewell jridgewell changed the title V2: Rename jQuery View hook methods v2: Rename jQuery View hook methods Jul 7, 2015
@akre54 akre54 added the change label Jul 7, 2015
@akre54
Copy link
Collaborator

akre54 commented Jul 7, 2015

Uh why? They're "private" here still. Other methods are exposed to plugins for overriding and they don't worry about naming either. The $ prefix looks ugly, imo.

@jridgewell
Copy link
Collaborator Author

They're hooks meant to be overridden, not private methods. Methods like #_ensureElement aren't meant to be overridden, and are subject to change in patch releases. Using the same naming convention for both doesn't seem like a smart idea.


I'm working on a PR that'll change #_ensureElement up a bit, which is what got me thinking of this.

@akre54
Copy link
Collaborator

akre54 commented Jul 7, 2015

But we've always exposed functions with the intention of allowing them to be overridden, either by plugin authors or end users. This is really no different than allowing an associations plugin to override get and set and expecting that to work. We shouldn't handle View's hooks any different than anywhere else in the code.

@jridgewell
Copy link
Collaborator Author

But we've always exposed functions with the intention of allowing them to be overridden, either by plugin authors or end users.

I don't see methods like Model#_validate and Collection#_removeModels, Collection#_onModelEvent, etc as publicly supported hook methods. That they're accessible on the prototype is due to code organization, not that they're meant for an end user to override. I'd argue that our private methods shouldn't be overridden ever: we're not obligated to keep them around between versions.

This is really no different than allowing an associations plugin to override get and set and expecting that to work.

#get and #set are both public methods with defined functionality. Changing them requires a new major.

We shouldn't handle View's hooks any different than anywhere else in the code.

I'm working on exposing our other hooks (Collection#_isModel springs to mind) publicly. But our internal methods shouldn't look like hook methods.

@braddunbar
Copy link
Collaborator

They're hooks meant to be overridden, not private methods.

Underscored methods are certainly meant to be overridden. Otherwise, we'd hide them in local scope, no? The distinction is that they're less stable and could be changed without the same notice as public API. My opinion would be that these methods fit that description.

@megawac
Copy link
Collaborator

megawac commented Jul 26, 2015

👎 this seems like an unnecessary reason to break code and overall poor change

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

Successfully merging this pull request may close these issues.

None yet

5 participants