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

Silent set functionality for objects #28

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Neogavin

Neogavin commented Feb 1, 2013

Been using Stapes for a project here at work, and was wondering why this wasn't a feature already: Implement silent set functionality for objects as well as individual values.

Because sometimes, I want to update a model without having it notify everything else right away.

Also showed an alternative way to handle the silent flag existing or not.

Update stapes.js
Implement silent update functionality for objects as well as individual values.
@contra

This comment has been minimized.

Show comment
Hide comment
@contra

contra Feb 2, 2013

Doesn't this already exist? I see tests for this too

contra commented Feb 2, 2013

Doesn't this already exist? I see tests for this too

@Neogavin

This comment has been minimized.

Show comment
Hide comment
@Neogavin

Neogavin Feb 2, 2013

http://jsfiddle.net/Neogavin/3WPYS/5/

Here's a jsfiddle that shows the problem exactly. If you set/update using an object, the silent flag is ignored.

Neogavin commented Feb 2, 2013

http://jsfiddle.net/Neogavin/3WPYS/5/

Here's a jsfiddle that shows the problem exactly. If you set/update using an object, the silent flag is ignored.

@hay

This comment has been minimized.

Show comment
Hide comment
@hay

hay Feb 4, 2013

Owner

Hey Neogavin, thanks for your pull request!

Right now the silent option is not just a argument for set, but also for push, remove and update. IMHO, adding the flag for objects in set would also require adding it for those three other functions. I'm not sure if the use case is big enough. Note that if you want, you can pretty easily monkeypatch Stapes to have the functionality you want using the Stapes.extend method.

Owner

hay commented Feb 4, 2013

Hey Neogavin, thanks for your pull request!

Right now the silent option is not just a argument for set, but also for push, remove and update. IMHO, adding the flag for objects in set would also require adding it for those three other functions. I'm not sure if the use case is big enough. Note that if you want, you can pretty easily monkeypatch Stapes to have the functionality you want using the Stapes.extend method.

@Neogavin

This comment has been minimized.

Show comment
Hide comment
@Neogavin

Neogavin Feb 5, 2013

That's how I'm currently using it on my project. I just think it'd be more consistent for the silent flag to always work, and not just in the case of setting/pushing/removing/updating one attribute. And its a small enough change that I could literally do it in a few minutes and commit it.

I can understand it not seeming like a very big use case right now, but this was something that threw me off for a while, and I am using it in a production environment, so I'm sure someone else will want this feature added in at some point. I could submit another pull request with the change already in it if needed.

Neogavin commented Feb 5, 2013

That's how I'm currently using it on my project. I just think it'd be more consistent for the silent flag to always work, and not just in the case of setting/pushing/removing/updating one attribute. And its a small enough change that I could literally do it in a few minutes and commit it.

I can understand it not seeming like a very big use case right now, but this was something that threw me off for a while, and I am using it in a production environment, so I'm sure someone else will want this feature added in at some point. I could submit another pull request with the change already in it if needed.

@contra

This comment has been minimized.

Show comment
Hide comment
@contra

contra Feb 6, 2013

I agree with @Neogavin it seems like inconsistent behavior for set to not obey the silent flag in a case as standard as this. If you want to initialize a bunch of properties on init but don't want it to trigger a bunch of events off (which seems like it is what you would want to do) it seems like overkill to write a bunch of .set calls instead of one

contra commented Feb 6, 2013

I agree with @Neogavin it seems like inconsistent behavior for set to not obey the silent flag in a case as standard as this. If you want to initialize a bunch of properties on init but don't want it to trigger a bunch of events off (which seems like it is what you would want to do) it seems like overkill to write a bunch of .set calls instead of one

Added in chaining functionality for on/off events.
Added in chaining functionality for on/off events.  Just returns the Stapes.subclass when you set an event to on/off.
@Neogavin

This comment has been minimized.

Show comment
Hide comment
@Neogavin

Neogavin Feb 8, 2013

Oh, shoot, that last commit was an error, I put it on the wrong branch.

Neogavin commented Feb 8, 2013

Oh, shoot, that last commit was an error, I put it on the wrong branch.

Take out chaining for now.
That was my mistake.
@hay

This comment has been minimized.

Show comment
Hide comment
@hay

hay Feb 8, 2013

Owner

Okay, the use case seems legit, and i agree that it's probably easy to implement. However, i'll only accept a pull request if:

  • You implement the feature for set, push, remove and update.
  • You add some simple unit tests (see test directory)
  • You add it to the docs.

I'll be closing this PR for now, if you got something, please open a new one. I'm looking forward to your contributions and thanks for all your comments on Stapes!

Owner

hay commented Feb 8, 2013

Okay, the use case seems legit, and i agree that it's probably easy to implement. However, i'll only accept a pull request if:

  • You implement the feature for set, push, remove and update.
  • You add some simple unit tests (see test directory)
  • You add it to the docs.

I'll be closing this PR for now, if you got something, please open a new one. I'm looking forward to your contributions and thanks for all your comments on Stapes!

@hay hay closed this Feb 8, 2013

hay added a commit that referenced this pull request Jul 9, 2013

@hay

This comment has been minimized.

Show comment
Hide comment
@hay

hay Jul 9, 2013

Owner

Well, i was a little harsh last time, it didn't actually take the time i thought to code it, so i rolled in silent functionality for objects in set in the latest release, v0.8.0, which i've released just now! Thanks for your suggestion and PR!

Owner

hay commented Jul 9, 2013

Well, i was a little harsh last time, it didn't actually take the time i thought to code it, so i rolled in silent functionality for objects in set in the latest release, v0.8.0, which i've released just now! Thanks for your suggestion and PR!

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