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

make select dumb #37

Merged
merged 4 commits into from
Sep 21, 2015
Merged

make select dumb #37

merged 4 commits into from
Sep 21, 2015

Conversation

dmfenton
Copy link
Contributor

This PR removes all logic from the select function except getting feature collections from the database.

This is a breaking change.

@dmfenton
Copy link
Contributor Author

cc @koopjs/dev

@dmfenton
Copy link
Contributor Author

@chelm I couldn't find the use of idFilter in any of the existing Koop providers. Can you show me where/how its used?

@chelm
Copy link
Contributor

chelm commented Sep 18, 2015

@dmfenton sure. It is super crucial and completely my fault for not docing it i guess. However, its also super concerning that it was removed, this would have been an issue pretty quickly I think. One sec.

@chelm
Copy link
Contributor

chelm commented Sep 18, 2015

@dmfenton its used in the Exporter for dealing with large data.

https://github.com/koopjs/koop/blob/master/lib/Exporter.js#L29-L44

This was introduced as an optimization in place of using offset & limit in selecting pages of data from the db. It could very well be moved into the Cache itself (which is probably where it belongs honestly as koop-pgcache is probably the only that would use it). your call...

@chelm
Copy link
Contributor

chelm commented Sep 18, 2015

also remember that typically providers will never call pgcache or any cache directly, they will always route through code in koop/lib in some way. So any breaking changes in the caches should be protected from the providers and handled more centrally.

@dmfenton
Copy link
Contributor Author

@dmfenton its used in the Exporter for dealing with large data.

Good looking out. Really glad you were watching to catch that one. I'll add it back. We can consider a more general solution in the future.

@chelm
Copy link
Contributor

chelm commented Sep 18, 2015

+1 for changelogs...

@dmfenton
Copy link
Contributor Author

And we're back

@dmfenton
Copy link
Contributor Author

@chelm @ngoldman any more changes to make?

@@ -295,6 +281,7 @@ module.exports = {
* @returns {string} sql
*/
createWhereFromSql: function (where, fields) {
if (where === '1=1') return where
Copy link
Contributor

Choose a reason for hiding this comment

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

smh esri

@ungoldman
Copy link
Contributor

doc looks good and tests are passing... lgtm. unless @chelm has ideas I say we push the green button.

@chelm
Copy link
Contributor

chelm commented Sep 21, 2015

lgtm 👶

ungoldman added a commit that referenced this pull request Sep 21, 2015
@ungoldman ungoldman merged commit e9d5f6d into master Sep 21, 2015
@ungoldman ungoldman deleted the refactor/select branch September 21, 2015 21: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.

3 participants