watch string comparator, cache spec, refactor configure #27

aaronmccall · Dec 5, 2014

closes #15

+1 on adding the comparator to the watch list if its a string.

I'm confused on what is the difference between the new spec.watch and spec.watched?

lukekarrys · Dec 5, 2014

+1 on adding the comparator to the watch list if its a string.

I'm confused on what is the difference between the new spec.watch and spec.watched?

My bad! I missed the spec.watched. I'll amend the PR.

aaronmccall · Dec 5, 2014

My bad! I missed the spec.watched. I'll amend the PR.

@aaronmccall

+1 @aaronmccall. Maybe squash the commits to include just the first relevant one?

lukekarrys · Dec 5, 2014

+1 @aaronmccall. Maybe squash the commits to include just the first relevant one?

Squashed!

aaronmccall · Dec 5, 2014

Squashed!

Does this need adjusting as well?

https://github.com/aaronmccall/ampersand-subcollection/blob/master/ampersand-subcollection.js#L109

legastero · Dec 5, 2014

Does this need adjusting as well?

https://github.com/aaronmccall/ampersand-subcollection/blob/master/ampersand-subcollection.js#L109

@legastero

Good call, @legastero! On it.

aaronmccall · Dec 5, 2014

Good call, @legastero! On it.

@legastero

Ok, @legastero and @lukekarrys, the problem as I see it is that we can't reset _watched back to the original spec.watched because we only have it in its unchanged state in the constructor at L13. This means that either we need a way to keep a reference to an unmodified version of the original spec.watched to re-initialize _watched with, or we need to allow configure (see L75) to re-initialize it, or both.

aaronmccall · Dec 5, 2014

Ok, @legastero and @lukekarrys, the problem as I see it is that we can't reset _watched back to the original spec.watched because we only have it in its unchanged state in the constructor at L13. This means that either we need a way to keep a reference to an unmodified version of the original spec.watched to re-initialize _watched with, or we need to allow configure (see L75) to re-initialize it, or both.

What about doing something like this?

diff --git a/ampersand-subcollection.js b/ampersand-subcollection.js
index 4fb13b5..ec6d0be 100644
--- a/ampersand-subcollection.js
+++ b/ampersand-subcollection.js
@@ -7,10 +7,10 @@ var slice = Array.prototype.slice;


 function SubCollection(collection, spec) {
-    spec || (spec = {});
+    this._spec = (spec || (spec = {}));
     this.collection = collection;
     this._reset();
-    this._watched = spec.watched || [];
+    this._watch(spec.watched);
     this._parseFilters(spec);
     this._runFilters();
     this.listenTo(this.collection, 'all', this._onCollectionEvent);
@@ -73,7 +73,12 @@ _.extend(SubCollection.prototype, Events, underscoreMixins, {
     //   limit: 20
     // }
     configure: function (opts, clear) {
-        if (clear) this._resetFilters();
+
+        if (clear) {
+            this._spec = {};
+            this._resetFilters();
+        }
+        this._watch(opts.watched);
         this._parseFilters(opts);
         this._runFilters();
     },
@@ -107,6 +112,7 @@ _.extend(SubCollection.prototype, Events, underscoreMixins, {
     _resetFilters: function () {
         this._filters = [];
         this._watched = [];
+        this._watch(this._spec.watched);
         this.limit = undefined;
         this.offset = undefined;
     },

aaronmccall · Dec 5, 2014

What about doing something like this?

diff --git a/ampersand-subcollection.js b/ampersand-subcollection.js
index 4fb13b5..ec6d0be 100644
--- a/ampersand-subcollection.js
+++ b/ampersand-subcollection.js
@@ -7,10 +7,10 @@ var slice = Array.prototype.slice;


 function SubCollection(collection, spec) {
-    spec || (spec = {});
+    this._spec = (spec || (spec = {}));
     this.collection = collection;
     this._reset();
-    this._watched = spec.watched || [];
+    this._watch(spec.watched);
     this._parseFilters(spec);
     this._runFilters();
     this.listenTo(this.collection, 'all', this._onCollectionEvent);
@@ -73,7 +73,12 @@ _.extend(SubCollection.prototype, Events, underscoreMixins, {
     //   limit: 20
     // }
     configure: function (opts, clear) {
-        if (clear) this._resetFilters();
+
+        if (clear) {
+            this._spec = {};
+            this._resetFilters();
+        }
+        this._watch(opts.watched);
         this._parseFilters(opts);
         this._runFilters();
     },
@@ -107,6 +112,7 @@ _.extend(SubCollection.prototype, Events, underscoreMixins, {
     _resetFilters: function () {
         this._filters = [];
         this._watched = [];
+        this._watch(this._spec.watched);
         this.limit = undefined;
         this.offset = undefined;
     },

@aaronmccall

@aaronmccall I think that looks good. Now that we're expanding the scope of this issue, we should probably go ahead and test all this behavior as well.

lukekarrys · Dec 5, 2014

@aaronmccall I think that looks good. Now that we're expanding the scope of this issue, we should probably go ahead and test all this behavior as well.

Agreed. I'll start writing tests.

aaronmccall · Dec 5, 2014

Agreed. I'll start writing tests.

Seems unexpected that anything, such as the _watched list would be maintained during a reset: https://github.com/AmpersandJS/ampersand-subcollection/pull/27/files#diff-ee4701d13a7c5fb628bd1061d96bb1d1R121

Then there's no way to truly reset and reconfigure.

Also, keeping and then extending original spec: https://github.com/AmpersandJS/ampersand-subcollection/pull/27/files#diff-ee4701d13a7c5fb628bd1061d96bb1d1R84 seems troublesome if you're really wanting to reconfigure things over time.

HenrikJoreteg · Dec 8, 2014

Seems unexpected that anything, such as the _watched list would be maintained during a reset: https://github.com/AmpersandJS/ampersand-subcollection/pull/27/files#diff-ee4701d13a7c5fb628bd1061d96bb1d1R121

Then there's no way to truly reset and reconfigure.

Also, keeping and then extending original spec: https://github.com/AmpersandJS/ampersand-subcollection/pull/27/files#diff-ee4701d13a7c5fb628bd1061d96bb1d1R84 seems troublesome if you're really wanting to reconfigure things over time.

@HenrikJoreteg

@HenrikJoreteg,

Regarding your first point: The _watched list is only maintained through _resetFilters, not through configure(config, true). I would be surprised if resetting filters reset anything else—non-filter watches, for instance. What is happening with the proposed change is that the where keys are being removed from _watched but explicitly configured spec.watched and comparator watches are maintained.
Regarding the second, I believe you may have missed the reinitialization of this._spec at https://github.com/AmpersandJS/ampersand-subcollection/pull/27/files#diff-ee4701d13a7c5fb628bd1061d96bb1d1R80

aaronmccall · Dec 8, 2014

@HenrikJoreteg,

Regarding your first point: The _watched list is only maintained through _resetFilters, not through configure(config, true). I would be surprised if resetting filters reset anything else—non-filter watches, for instance. What is happening with the proposed change is that the where keys are being removed from _watched but explicitly configured spec.watched and comparator watches are maintained.
Regarding the second, I believe you may have missed the reinitialization of this._spec at https://github.com/AmpersandJS/ampersand-subcollection/pull/27/files#diff-ee4701d13a7c5fb628bd1061d96bb1d1R80

I did miss the re-init of this._spec but I don't see in this code how you'd ever reset all the things?

HenrikJoreteg · Dec 8, 2014

I did miss the re-init of this._spec but I don't see in this code how you'd ever reset all the things?

Also, i think perhaps clearFilters is causing some confusion. Both where and limit and offset are really just "filters" for all practical purposes.

HenrikJoreteg · Dec 8, 2014

Also, i think perhaps clearFilters is causing some confusion. Both where and limit and offset are really just "filters" for all practical purposes.

The steps in the proposed configure and _resetFilters changes ensure that configure(config, true) actually resets everything where now it doesn't since in the current version, we have the issue mentioned in #11, where the only way to reset the comparator is to manually remove it.

In the proposed changes, given that _spec is reset to an empty object (L80), comparator is being deleted (L81), _resetFilters is being run (L82)—removing all _filters and limit and offset, and rebuilding _spec from the passed in opts (L84), everything should be reset, shouldn't it?

It also ensures that you don't accidentally remove watches that should be maintained when _resetFilters is called.

aaronmccall · Dec 8, 2014

The steps in the proposed configure and _resetFilters changes ensure that configure(config, true) actually resets everything where now it doesn't since in the current version, we have the issue mentioned in #11, where the only way to reset the comparator is to manually remove it.

In the proposed changes, given that _spec is reset to an empty object (L80), comparator is being deleted (L81), _resetFilters is being run (L82)—removing all _filters and limit and offset, and rebuilding _spec from the passed in opts (L84), everything should be reset, shouldn't it?

It also ensures that you don't accidentally remove watches that should be maintained when _resetFilters is called.

Yeah, using configure({}, true) clears things, but the semantics of the rest of it is what seems like it could be better.

I think we need a method that fully resets everything and I don't think the only way to trigger this behavior should be by passing true to configure.

clearFilters was supposed to be that, but the name doesn't match that intent.

I wonder if we shouldn't do this:

clearFilters - removes any of the previously registered filter functions and where rules (since those are just declarative filters). It also removes relevant things from watched list. Basically, reset everything other than comparator.
change _reset to reset and have it do what it says on the box: "reset all the things" including comparator.
I don't think this code belongs in a reset.
I think this code should just happen as part of the reset.

Also, rather than just treating it as a generic watched i think the case of the string-based comparator, might make more sense to just handle separately by including in the list here: https://github.com/aaronmccall/ampersand-subcollection/blob/master/ampersand-subcollection.js#L220 then we don't have to maintain it as a "special" watched property.

HenrikJoreteg · Dec 8, 2014

Yeah, using configure({}, true) clears things, but the semantics of the rest of it is what seems like it could be better.

I think we need a method that fully resets everything and I don't think the only way to trigger this behavior should be by passing true to configure.

clearFilters was supposed to be that, but the name doesn't match that intent.

I wonder if we shouldn't do this:

clearFilters - removes any of the previously registered filter functions and where rules (since those are just declarative filters). It also removes relevant things from watched list. Basically, reset everything other than comparator.
change _reset to reset and have it do what it says on the box: "reset all the things" including comparator.
I don't think this code belongs in a reset.
I think this code should just happen as part of the reset.

Also, rather than just treating it as a generic watched i think the case of the string-based comparator, might make more sense to just handle separately by including in the list here: https://github.com/aaronmccall/ampersand-subcollection/blob/master/ampersand-subcollection.js#L220 then we don't have to maintain it as a "special" watched property.

Now we're getting somewhere! I'll work through this and see where it takes us!

aaronmccall · Dec 8, 2014

Now we're getting somewhere! I'll work through this and see where it takes us!

yeah, sorry my first attempts at articulating my feedback were terrible :) Thanks for your patience.

HenrikJoreteg · Dec 8, 2014

yeah, sorry my first attempts at articulating my feedback were terrible :) Thanks for your patience.

HenrikJoreteg · Dec 9, 2014

HenrikJoreteg reviewed Dec 9, 2014

View changes

HenrikJoreteg · Dec 9, 2014

ampersand-subcollection.js

@@ -144,9 +151,6 @@ _.extend(SubCollection.prototype, Events, underscoreMixins, {
        if (spec.filters) {
            spec.filters.forEach(this._addFilter, this);
        }
-        if (spec.comparator) {


Any particular reason you removed this from here? now it's duplicated in the constructor and configure instead. The idea for parseFilters is split out so you can can either create or call configure with the same object structure and everything works.

Maybe it should be called _parseSpec and initialize _watched, _filters, and comparator?

makes sense to me

Will also need documentation update to match these changes.

HenrikJoreteg · Dec 9, 2014

Will also need documentation update to match these changes.

@HenrikJoreteg

@HenrikJoreteg, if we're good to go with the code changes, I can update docs to match.

aaronmccall · Dec 9, 2014

@HenrikJoreteg, if we're good to go with the code changes, I can update docs to match.

HenrikJoreteg · Dec 9, 2014

HenrikJoreteg reviewed Dec 9, 2014

View changes

test/main.js

comparator can be a key string; watch it if it is

edaf6e4

bear added request enhancement labels Dec 5, 2014

aaronmccall changed the title from add string comparator and declared watches to _watched to add string comparator to _watched Dec 5, 2014

aaronmccall changed the title from add string comparator to _watched to watch string comparator, cache spec, refactor configure Dec 8, 2014

aaronmccall referenced this pull request Dec 8, 2014
Closed
There's no way to reset comparator #11

Aaron McCall added some commits Dec 8, 2014

rename _reset to reset; truly reset

acaa2ca

monitor changes to comparator key more simply

93b82d6

Aaron McCall added some commits Dec 8, 2014

make _unwatch work like _watch

b5789d8

cache filtered models before slicing; ref #6

0eddafe

updated tests for changes

230e5e3

_parseFilters --> _parseSpec and init things there

4b2f6e8

make sure 'reset' fires events.
use '_resetFilters' in 'configure' instead of 'reset'. add tests for 'reset' add tests for 'clearFilters' removing 'comparator' should set it back to parent's 'comparator' includes #27

9fa7747

HenrikJoreteg referenced this pull request Dec 9, 2014
Merged
Refactor configure #28

Aaron McCall added some commits Dec 11, 2014

move comparator reset to _resetFilters
- put it behind a param flag - corrects bug with configure({}, true) not resetting comparator

a20fe37

docs update

eb1697b

aaronmccall closed this Dec 12, 2014

AmpersandJS/ampersand-subcollection

Join GitHub today

watch string comparator, cache spec, refactor configure #27

Conversation

aaronmccall commented Dec 5, 2014

This comment has been minimized.

lukekarrys commented Dec 5, 2014

bear added request enhancement labels Dec 5, 2014

This comment has been minimized.

aaronmccall commented Dec 5, 2014

aaronmccall changed the title from add string comparator and declared watches to _watched to add string comparator to _watched Dec 5, 2014

This comment has been minimized.

lukekarrys commented Dec 5, 2014

This comment has been minimized.

aaronmccall commented Dec 5, 2014

This comment has been minimized.

legastero commented Dec 5, 2014

This comment has been minimized.

aaronmccall commented Dec 5, 2014

This comment has been minimized.

aaronmccall commented Dec 5, 2014

This comment has been minimized.

aaronmccall commented Dec 5, 2014

This comment has been minimized.

lukekarrys commented Dec 5, 2014

This comment has been minimized.

aaronmccall commented Dec 5, 2014

Aaron McCall added some commits Dec 5, 2014

aaronmccall changed the title from add string comparator to _watched to watch string comparator, cache spec, refactor configure Dec 8, 2014

This comment has been minimized.

HenrikJoreteg commented Dec 8, 2014

This comment has been minimized.

aaronmccall commented Dec 8, 2014

aaronmccall referenced this pull request Dec 8, 2014

This comment has been minimized.

HenrikJoreteg commented Dec 8, 2014

This comment has been minimized.

HenrikJoreteg commented Dec 8, 2014

This comment has been minimized.

aaronmccall commented Dec 8, 2014

This comment has been minimized.

HenrikJoreteg commented Dec 8, 2014

This comment has been minimized.

aaronmccall commented Dec 8, 2014

This comment has been minimized.

HenrikJoreteg commented Dec 8, 2014

Aaron McCall added some commits Dec 8, 2014

Aaron McCall added some commits Dec 8, 2014

HenrikJoreteg reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

HenrikJoreteg commented Dec 9, 2014

This comment has been minimized.

aaronmccall commented Dec 9, 2014

HenrikJoreteg reviewed Dec 9, 2014 View changes

HenrikJoreteg referenced this pull request Dec 9, 2014

Aaron McCall added some commits Dec 11, 2014

aaronmccall closed this Dec 12, 2014

HenrikJoreteg reviewed Dec 9, 2014

View changes

HenrikJoreteg reviewed Dec 9, 2014

View changes