Skip to content

Null-safety implementation #25

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

Merged
merged 4 commits into from
Nov 22, 2021
Merged

Null-safety implementation #25

merged 4 commits into from
Nov 22, 2021

Conversation

lukas-h
Copy link
Member

@lukas-h lukas-h commented Nov 21, 2021

Hi,
this is my attempt at null-safety.

GeoJson serialization/deserialization

I tried to follow the official GeoJSON RFC-standard as good as possible, there is still some ambiguity left on how to interpret and translate it into Dart.

  • The attributes of CoordinateType and it's child-classes BBox and Position are non-nullable.

    abstract class CoordinateType implements Iterable<num> {

  • RFC 7946 – 3.1.1 The optional third attribute (alt) for Position is also non-nullable and by default == 0. I'm not sure if this is a good way of dealing with it?!

  • RFC 7946 – 3.1.2 and following: I made the coordinates attribute for Point, MultiPoint, LineString, MultiLineString, Polygon and MultiPolygon nullable. But I only made the "top-level" type nullable: this means, either you assign null to coordinates or you have to provide it entirely null-safe.

  • But should it be nullable? Let's vote on that.

    "GeoJSON processors MAY interpret Geometry objects with empty "coordinates" arrays as null objects." RFC 7946 – 3.1

  • RFC 7946 – 3.1.8 similarly to the other types above, I made the geometries attribute for GeometryCollection nullable

  • RFC 7946 – 3.2 Feature: the geometry is also nullable

  • RFC 7946 – 3.3 FeatureCollection: the features attribute "can be empty" according to the RFC-standard, so I chose to treat it as nullable.

Ported "Components" from Turf.js

For functions such as midpoint, destination, etc. we have two implementations: "raw" (which takes just the Position) and the other one with Points, which just passes Point.position attribute to the "raw" function.

Point midpoint(Point point1, Point point2) => Point(

Position midpointRaw(Position point1, Position point2) {

I don't really think using the bang(!)-operator is a good way of dealing with it, because we give the responsibility to check for null again to the user, which is kind of opposed to the whole idea of null-safety.

coordinates: midpointRaw(point1.coordinates!, point2.coordinates!),

Please @baparham @tobrun, review and correct the migration for the meta functions, ...


Also: I have still an error with json_serializable, which has some kind of type error, I guess it has something to do with the nested-generic types..

When I try to run: dart run build_runner build, this is the output:

...
[SEVERE] json_serializable:json_serializable on lib/src/geojson.dart:

UnimplementedError: (TypeParameterTypeImpl) T?
[INFO] Running build completed, took 3.8s
[INFO] Caching finalized dependency graph completed, took 36ms
[SEVERE] Failed after 3.8s

@tobrun @baparham if you could take a look and help me with all of the decisions? 🙂

@lukas-h lukas-h assigned baparham and lukas-h and unassigned baparham Nov 21, 2021
@lukas-h lukas-h added the help wanted Extra attention is needed label Nov 21, 2021
@tobrun
Copy link
Collaborator

tobrun commented Nov 21, 2021

RFC 7946 – 3.1.1 The optional third attribute (alt) for Position is also non-nullable and by default == 0. I'm not sure if this is a good way of dealing with it?!

Capturing from the spec:

Altitude or elevation MAY be included as an optional third element.

To me optional equals nullable. The issue with defaulting to 0 means you can't differentiate when a value is uninitialized or actually 0.

RFC 7946 – 3.1.2 and following: I made the coordinates attribute for Point, MultiPoint, LineString, MultiLineString, Polygon and MultiPolygon nullable. But I only made the "top-level" type nullable: this means, either you assign null to coordinates or you have to provide it entirely null-safe.

My personal opinion would be to make these non-nullable. Is their any language in the specification that goes against this? Conceptually a GeoJSON type is determined by the underlying geometry, if their isn't any geometry, is it really an instance of it?

RFC 7946 – 3.3 FeatureCollection: the features attribute "can be empty" according to the RFC-standard, so I chose to treat it as nullable.

👍

@lukas-h
Copy link
Member Author

lukas-h commented Nov 21, 2021

Conceptually a GeoJSON type is determined by the underlying geometry, if their isn't any geometry, is it really an instance of it?

RFC 7946 – 3.1

GeoJSON processors MAY interpret Geometry objects with empty "coordinates" arrays as null objects.

And for Features [I wanted to treat Feature, FeatureCollection, GeometryCollection and the geometry types (Point, Polygon, ...) the same way nullability-wise]:

A Feature object has a member with the name "geometry". 
The value of the geometry member SHALL be either a Geometry object as defined above or,
in the case that the Feature is unlocated, a JSON null value.

RFC 7946 – 3.2

@lukas-h
Copy link
Member Author

lukas-h commented Nov 21, 2021

To me optional equals nullable. The issue with defaulting to 0 means you can't differentiate when a value is uninitialized or actually 0.

Totally makes sense, but I have to see how that will complicate the implementation of bounding boxes.

Following the RFC, they can have either 4 (without altitude, 2D) or 6 parameters (with alt, 3D), see here.

I had implemented it before as 3D by default.

]) : super([lng1, lat1, alt1, lng2, lat2, alt2]);

Copy link
Member

@baparham baparham left a comment

Choose a reason for hiding this comment

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

a few thoughts

@lukas-h lukas-h marked this pull request as ready for review November 22, 2021 19:25
@lukas-h lukas-h requested a review from baparham November 22, 2021 19:25
@lukas-h
Copy link
Member Author

lukas-h commented Nov 22, 2021

I think I now have implemented everything necessary for the migration
and I would really appreciate to get reviews on my latest changes 😄

@@ -6,155 +6,126 @@ part of 'geojson.dart';
// JsonSerializableGenerator
Copy link
Member

Choose a reason for hiding this comment

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

who or what changes this file? Is it generated code somehow or is it used to generate code?

Copy link
Member Author

Choose a reason for hiding this comment

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

It‘s auto-generated by json_serializable.

This package: https://pub.dev/packages/json_serializable
A code generator for JSON (de)serialization.

Copy link
Member

@baparham baparham left a comment

Choose a reason for hiding this comment

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

Looks like you've addressed our questions and decisions, nice work!

@lukas-h
Copy link
Member Author

lukas-h commented Nov 22, 2021

Can I go ahead and merge it?

@baparham @tobrun any objections?

@baparham
Copy link
Member

History is reversible. At least here it is 😂

@lukas-h lukas-h merged commit 31f9e21 into main Nov 22, 2021
@lukas-h lukas-h deleted the null-safety-support branch November 22, 2021 22:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants