koopjs / koop-pgcache

PostGIS cache for Koop.
Other
4 stars 4 forks source link

make select dumb #37

Closed dmfenton closed 9 years ago

dmfenton commented 9 years ago

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

This is a breaking change.

dmfenton commented 9 years ago

cc @koopjs/dev

dmfenton commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

+1 for changelogs...

dmfenton commented 9 years ago

And we're back

dmfenton commented 9 years ago

@chelm @ngoldman any more changes to make?

ungoldman commented 9 years ago

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

chelm commented 9 years ago

lgtm :baby: