Skip to content

Conversation

@GuyMograbi
Copy link

finally I can go to sleep, this has been haunting me.

We can simplify things by using the fact that scope has a prototype inheritance.
 into fix_inheritance_algorithm

Conflicts:
	app/scripts/app.js
 into fix_inheritance_algorithm

Conflicts:
	app/scripts/app.js
@sefininio
Copy link
Contributor

How will this code handle isolated scopes? They do not inherit from $rootScope prototypically.

@GuyMograbi
Copy link
Author

The question is wrong..

It has nothing to do with $rootScope. $rootScope is just a way for me to
reach the prototype. (which oddly is in the rootScope.js source file, but
this is just semantics)

They share the same prototype.

https://github.com/angular/angular.js/blob/master/src/ng/rootScope.js (lines
~221)

specifically line 218
...
if (isolate) {
child = new Scope();
...
}
...

I am changing Scope, not $rootScope.. so it will apply.
It will even apply if the scopes were already created! how awesome is that?

Here is a plunker to demonstrate
http://plnkr.co/edit/ayat5H1daSxUDr900V6f?p=preview

On Mon, Oct 12, 2015 at 9:24 AM, Sefi Ninio [email protected]
wrote:

How will this code handle isolated scopes? They do not inherit from
$rootScope prototypically.


Reply to this email directly or view it on GitHub
#19 (comment)
.

@sefininio
Copy link
Contributor

This looks very elegant (and the ability to affect scopes already created is indeed awesome!)
I'll be happy to merge the PR.

Though I'm not sure it actually works. Your plunk demonstrates prototype inheritance - which, not surprisingly, works.

However, before I merge I need to be sure that it does not break the functionality.
It is common practice, when submitting a PR to also include a test or, alternately, specify which existing test proves nothing is broken.

So, help me do the merge :)

@GuyMograbi
Copy link
Author

The tests already exists - you wrote them.

This is a change to existing functionality. Everything should work the
same.
So the tests passing is what you need.

Regarding my previous answer, I just answered the question. insulting
people in public is not a great motivator for contributions.

On Tue, Oct 13, 2015 at 9:16 AM, Sefi Ninio [email protected]
wrote:

This looks very elegant (and the ability to affect scopes already created
is indeed awesome!)
I'll be happy to merge the PR.

Though I'm not sure it actually works. Your plunk demonstrates prototype
inheritance - which, not surprisingly, works.

However, before I merge I need to be sure that it does not break the
functionality.
It is common practice, when submitting a PR to also include a test or,
alternately, specify which existing test proves nothing is broken.

So, help me do the merge :)


Reply to this email directly or view it on GitHub
#19 (comment)
.

@sefininio
Copy link
Contributor

I'm not sure exactly what was insulting in what I wrote.
You expect me to merge the PR without first verifying it's working (or not breaking).
It is YOUR job to convince, not mine.
Also, you can't fix what is not broken. The PR is a refactor at best, and it's title is misleading.

You wanna contribute? great! there's an acceptable way to do so. Yours is just rude.

However, I'll review and decide on the merge.

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

Successfully merging this pull request may close these issues.

3 participants