Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Simplify watchGroupFn assignment using ternary #7619

Closed
wants to merge 1 commit into from
Closed

Simplify watchGroupFn assignment using ternary #7619

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/ng/rootScope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,11 +400,7 @@ function $RootScopeProvider(){

deregisterFns.push(self.$watch(watchGroupFn, function () {
listener(newValues, oldValues, self);
if (unwatchCount === 0) {
watchGroupFn.$$unwatch = true;
} else {
watchGroupFn.$$unwatch = false;
}
watchGroupFn.$$unwatch = unwatchCount === 0 ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

you could go even further and say watchGroupFn.$$unwatch = !!!unwatchCount;

But I'm not sure if this kind of thing is going to get merged, because the style guide is pretty clear about preferring single expressions per line, and not necessarily approving of ternary expressions.

I'll see if we can do this though

Copy link
Author

Choose a reason for hiding this comment

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

I did think about something similar... such as !(!!unwatchCount); to make things a little less crazy ;) but yes... okay and thanks :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@toddmotto I think #7486 (comment) is probably showing that this isn't going to land, even if it's concise.

Minifiers could possibly do something cute like this, though.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure which comment, and okay! :)

Sent from my iPhone

On 28 May 2014, at 22:19, Caitlin Potter notifications@github.com wrote:

In src/ng/rootScope.js:

@@ -400,11 +400,7 @@ function $RootScopeProvider(){

     deregisterFns.push(self.$watch(watchGroupFn, function () {
       listener(newValues, oldValues, self);

Minifiers could possibly do something cute like this, though.

—
Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

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

I specifically linked to one of Igor's review comments from the PR which implemented this --- @lgalfaso originally used a similar expression, and was asked to replace it with if / else blocks.

Copy link
Author

Choose a reason for hiding this comment

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

Ahh okay. I don't think the GitHub mobile on iOS takes me to the comment in that case. Will check out on desktop!

Sent from my iPhone

On 28 May 2014, at 22:40, Caitlin Potter notifications@github.com wrote:

In src/ng/rootScope.js:

@@ -400,11 +400,7 @@ function $RootScopeProvider(){

     deregisterFns.push(self.$watch(watchGroupFn, function () {
       listener(newValues, oldValues, self);

  •      if (unwatchCount === 0) {

  •        watchGroupFn.$$unwatch = true;

  •      } else {

  •        watchGroupFn.$$unwatch = false;

  •      }

  •      watchGroupFn.$$unwatch = unwatchCount === 0 ? true : false;
    I specifically linked to one of Igor's review comments from the PR which implemented this --- @lgalfaso originally used a similar expression, and was asked to replace it with if / else blocks.

—
Reply to this email directly or view it on GitHub.

}));

return function deregisterWatchGroup() {
Expand Down