-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Strongly type geomEach function to GeoJSONObject #29
Conversation
Closes #29 |
tests passed locally and in ci, so I'll see if I can at least refactor this to be more maintainable/readable |
Question for discussion: How critical should we consider the comment about speed an efficiency in the geomEach turf.js implementation? I'm not sure if turf.js has any benchmarks out there for measuring performance that we could run against our dart implementation to ensure we maintain performance. The only thing I think I'd change is to basically have a few more |
Maybe we could create a similar benchmark suite, similar to: https://github.com/Turfjs/turf/blob/master/packages/turf-meta/bench.js I found this package for example: |
30ed90d
to
e8082ac
Compare
I rebased this on top of #25 and squashed the commits down to one |
This commit also renames Geometry to GeometryObject
c2bbe2d
to
176c736
Compare
lib/src/meta.dart
Outdated
Map<String, dynamic> featureProperties, | ||
BBox? featureBBox, | ||
dynamic featureId) { | ||
switch (currentGeometry.type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In PR #33, I used the is
operator instead of checking the .type
attribute in a switch case, which could save us some lines of code. Don't know if type checking is more efficient than this.
like this:
if(currentGeometry is GeometryType) {
// ...
} else if(currentGeometry is GeometryCollection) {
// ...
} else {
throw Exception('Unknown Geometry Type');
}
lib/src/meta.dart
Outdated
} | ||
} | ||
|
||
void _runGeomEachCallbacks( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tried to merge this function with _forEachGeomInGeometryObject
in my PR #33, please give me a feedback.
No description provided.