Skip to content
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

don't apply additional filters to the result that is returned from the remote endpoint #1762

Conversation

jochenberger
Copy link
Contributor

See #1352, #1214, #664

@jochenberger
Copy link
Contributor Author

Not sure what to do with those test. I'd say they should be removed, because they don't make sense anymore.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 92.822% when pulling 29c2b76 on jochenberger:dont-filter-result-from-endpoint into 1d15068 on JedWatson:master.

@agirton
Copy link
Collaborator

agirton commented Jun 3, 2017

Hi @jochenberger thank you for your contribution. Instead of removing them the default could be false. This way if users still want this behavior they can opt in. This feels like a breaking change though. Let me give this a bit more thought.

@agirton
Copy link
Collaborator

agirton commented Jun 6, 2017

Hi @jochenberger removing this behavior doesn't seem the right thing to do so can you update the PR to just default to false? We will need to communicate this in the docs as well.

@jochenberger
Copy link
Contributor Author

I updated the PR. Any suggestions on how the docs should be updated? I assume it should be mentioned in the "Async options" section.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 92.97% when pulling 4514509 on jochenberger:dont-filter-result-from-endpoint into 1d15068 on JedWatson:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 92.97% when pulling 4514509 on jochenberger:dont-filter-result-from-endpoint into 1d15068 on JedWatson:master.

@JedWatson
Copy link
Owner

Hey @jochenberger sorry for the runaround on this one - I'm not inclined to change the default behaviour before the next major version, as it's a big breaking change for anyone using the component and expecting the current behaviour.

Going to close this (aiming to get all PRs merged or closed) but will definitely consider less opinionated defaults in the next major version.

Thanks!

@JedWatson JedWatson closed this Nov 27, 2017
JedWatson added a commit that referenced this pull request Nov 27, 2017
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