Skip to content

Unwrap value in isObservableArray #17

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

Merged
merged 3 commits into from
Aug 2, 2016
Merged

Unwrap value in isObservableArray #17

merged 3 commits into from
Aug 2, 2016

Conversation

marcovdb
Copy link
Contributor

@marcovdb marcovdb commented Jul 31, 2016

This will allow the extender to work on a computed observable as well (as long at it's an array, which makes sense).

This will allow the extender to work on a computed observable as well.
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 96.429% when pulling 9d1ec68 on marcovdb:patch-1 into 162882f on ErikSchierboom:master.

@guilhermewaess
Copy link
Contributor

guilhermewaess commented Jul 31, 2016

@marcovdb Nice work, by the way looks like you can write some tests for this feature, what you think? ;)
But is just a advise <- this turns more easy to @ErikSchierboom accept the PR

@ErikSchierboom
Copy link
Owner

@marcovdb Love it! As @guilhermewaess mentioned, having one or more tests for this feature would be super helpful. Would you be willing to add a test?

@marcovdb
Copy link
Contributor Author

marcovdb commented Aug 1, 2016

Admittedly I don't have much experience with automated tests, but I'll see what I can do!

@ErikSchierboom
Copy link
Owner

@marcovdb Just give me a shout if you need any help!

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.2%) to 95.402% when pulling da103c2 on marcovdb:patch-1 into 162882f on ErikSchierboom:master.

@marcovdb
Copy link
Contributor Author

marcovdb commented Aug 1, 2016

Okay, I added a few unit tests. I'm not sure why this has actually decreased the coverage but as I said, I'm kind of new at this.

@ErikSchierboom ErikSchierboom merged commit e6a6970 into ErikSchierboom:master Aug 2, 2016
@ErikSchierboom
Copy link
Owner

@marcovdb Thanks for adding the tests. This is great work! I've just merged this PR and released a new version (0.4.0) that includes this improvement. Thanks again!

@marcovdb marcovdb deleted the patch-1 branch October 31, 2016 22:50
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.

4 participants