Potential breaking changes for elm-geometry 1.0

[ianmackenzie]

This is a meta-issue for potential breaking changes to make for an elm-geometry 1.0 version. Feel free to post suggestions in the comments and I'll edit this issue text.

Module naming

I used prefixed module names like OpenSolid.Point2d to avoid potential naming conflicts with modules from other packages, but based on elm/compiler#1625 it seems that this might be considered "silly module renaming". Should the prefixes be removed? I think they should remain in a few cases like OpenSolid.Geometry.Decode, but otherwise module names could be switched to just plain Point2d, Triangle2d etc. What about the Scalar and Interval modules? Those are fairly generic names, so might benefit from staying as OpenSolid.Scalar and OpenSolid.Interval...but then that feels a bit inconsistent/arbitrary. Rename to Scalar1d and Interval1d and just lay claim to all module names ending in 1d, 2d or 3d?

Switching to non-prefixed names would be more consistent with other Elm packages - elm-community/elm-test uses top-level Expect, Fuzz and Test modules, mdgriffith/style-elements uses top-level Style and Element modules, terezka/elm-plot uses a top-level Plot module, etc.

Additionally, if the non-prefixed names don't work out and run into conflicts with other packages, then these conflicts will provide additional data points for elm/compiler#1625.

Package split

This is tracked in a separate issue, but splitting this package into two smaller packages would certainly be a major version change (although it shouldn't actually require any code changes, just modifications to dependencies in elm-package.json).

Moves/renames

• It would probably make sense to have SweptAngle as its own module now that Arc2d and EllipticalArc2d both use the same concept (currently just two independent but identical Arc2d.SweptAngle and EllipticalArc2d.SweptAngle types). This would likely mean adding an Internal.SweptAngle type and just exposing SweptAngle.smallPositive etc.; the Arc2d and EllipticalArc2d modules could then pattern-match on the Internal.SweptAngle constructors. (Directly referring to stuff like SweptAngle.SmallPositive feels a bit weird, and I can't think of any cases where it would be useful for end-user code to pattern-match on SweptAngle values.) On the other hand, if the OpenSolid prefix is removed, then it might not make sense to have a top-level SweptAngle module (unlikely to conflict with modules from other packages, but a little less obvious what package it comes from than something like Point3d).
• I find I often want to write Direction2d.toAngle instead of Direction2d.angle. angle is consistent with other functions like components and coordinates in that it treats the returned angle as a property of the direction ("the angle of the direction") instead of an alternate representation ("the direction converted to an angle"); I wouldn't want to have Point2d.toTuple or Point2d.toCoordinates, for example, but somehow Direction2d.toAngle feels more natural than Direction2d.angle. Similarly should SketchPlane3d.plane be SketchPlane3d.toPlane? Does it make more sense to talk about "the plane of a sketch plane" or "converting a sketch plane to a plane"?
• It may be more clear to rename some or all of the with functions to have more descriptive names. In some cases like Axis#d.fromOriginAndDirection this might also imply switching from a single record argument to multiple 'plain' arguments. Possible renames:
  • Frame2d.with to Frame2d.fromOriginAndXDirection (and add Frame2d.fromOriginAndYDirection)
  • Frame3d.with to Frame3d.fromOriginAndZDirection (and add Frame3d.fromOriginAndXDirection, Frame3d.fromOriginAndYDirection)
  • Vector#d.with to Vector#d.fromLengthAndDirection
  • BoundingBox#d.with to BoundingBox#d.fromExtrema
  • Interval.with to Interval.fromExtrema
  • Axis#d.with to Axis#d.fromOriginAndDirection
  • Plane3d.with to Plane3d.fromPointAndNormal
  • Arc2d.with to Arc2d.fromCenterAndStart
  • Circle#d.with to Circle#d.fromCenterAndRadius (although Circle3d also requires an axialDirection parameter...)
  • Sphere3d.with to Sphere3d.fromCenterAndRadius
  • Direction3d.with to Direction3d.fromSphericalAngles
  • Ellipse2d.with to Ellipse2d.fromCenter, or perhaps change to Ellipse2d.centeredOn : Frame2d -> { xRadius : Float, yRadius : Float } -> Ellipse2d
  • EllipticalArc2d.with to EllipticalArc2d.fromCenter, or perhaps change to EllipticalArc2d.centeredOn : Frame2d -> { xRadius : Float, yRadius : Float, startAngle : Float, endAngle : Float } -> EllipticalArc2d
  • SketchPlane3d.with to SketchPlane3d.fromPointAndNormal
  • Rectangle2d.with to Rectangle2d.axisAligned

Switch from Maybe to Result

Currently there are several constructor/factory functions such as Arc2d.fromEndpoints that return Maybe values if construction fails (invalid arguments given, no solution found, etc.). Ideally many of these should be switched to return Result values instead with a custom error type that indicates the failure reason; for example, @folkertdev's proposal for EllipticalArc2d.fromEndpoints:

type ArcError
    = IdenticalStartAndEndpoint -- omit this segment
    | RadiusIsZero -- (i.e. endpoint == center) draw straight line segment 
    | NoSolution -- maybe point to docs on how svg deals with this

One possible modification: change the NoSolution case to actually include the 'best approximation' elliptical arc with X and Y radius scaled to make arc construction possible. This way attempting to construct an ellipse with invalid/impossible radii is an Err, but the user can explicitly choose to fall back to the scaled ellipse if they want (which is what browser SVG engines do, since that's what's mandated by the SVG spec).

[tomek-he-him]

Hey Ian, dropping a bunch of thoughts on those points that I have an opinion on. Hoping they’ll be helpful!

 

Module naming

This makes a ton of sense! As well as being in line with the patterns emerging within the community as you pointed out, it’ll also make it work more smoothly with tooling like atom-elmjutsu (and possibly other tools that were designed in line with the community standard).

 

Moves/renames

• I find I often want to write Direction2d.toAngle instead of Direction2d.angle. […]

In my own code, I almost always use the Thing.fromSomething and Thing.toSomething convention for conversions between value types or units. It goes in line with the core library: toInt, Dict.toList, Dict.fromList and many many more. It also reads very well both in pipelines and in conventional function syntax:

myAxis
    |> Axis2d.direction
    |> Direction2d.toAngle

Direction2d.toAngle myDirection

The convention works well for converting one value to another, but not for extracting a smaller part of a value. For example, Axis2d.direction and Point3d.xCoordinate are awesome as they are. 👍

 

• It may be more clear to rename some or all of the with functions to have more descriptive names.

Oh man, that makes a ton of sense! Goes totally in line with the point I made above about converting values.

Just curious, when moving away from a .with to a descriptive function like Circle.fromCenterAndRadius, would you prefer to keep the current record-based API?

Circle.fromCenterAndRadius { center = center, radius = radius }

Or take advantage of the descriptive naming to make the API more lightweight?

Circle.fromCenterAndRadius center radius

 

Switch from Maybe to Result

Sounds super sensible 👍 👍 👍

[ianmackenzie]

Thanks for the comments Tomek!

For the module renaming, one of the tricks is what to do with some of the more generic module names that are more likely to cause conflicts...I just edited the description to expand on this a bit, but here's the relevant text:

What should happen to the Scalar and Interval modules? Those are fairly generic names, so might benefit from staying as OpenSolid.Scalar and OpenSolid.Interval...but then that feels a bit inconsistent/arbitrary. Rename to Scalar1d and Interval1d and just lay claim to all module names ending in 1d, 2d or 3d?

I hadn't thought at all about the impact on editor autocomplete etc., that's a really good point! (I tend to just use Sublime myself with a very minimal set of plugins, pretty much just syntax highlighting and elm-format on save.) The module renaming question might be one to open up to the broader community via a post to the 'Request Feedback' topic on the Elm Discourse.

As for making some of the with functions more lightweight, it's definitely a good suggestion - I'd have to play around a bit and see what worked well. It might be that some relatively simple cases like Axis3d.fromOriginAndDirection could be switched to more lightweight versions, but for some like EllipticalArc2d.fromEndpoints (6 arguments) it might make sense to keep taking records. There are also a couple cases already that use a hybrid approach, like Arc3d.around which takes an Axis3d as the first argument but then a record for the remaining arguments. The idea was that the first argument is obvious from the function name, and it also formats pretty nicely with elm-format:

Arc3d.around Axis3d.z
    { startPoint =
        Point3d.fromCoordinates ( 1, 1, 0 )
    , sweptAngle = degrees 90
    }

It could be that things like EllipticalArc2d.fromEndpoints could use a similar pattern, for example:

EllipticalArc2d.fromEndpoints startPoint endPoint
    { xDirection = Direction2d.x
    , xRadius = 2
    , yRadius = 1
    , sweptAngle = EllipticalArc2d.smallPositive
    }

Making things more lightweight is good but I don't want to give up too much clarity/explicitness to get there!

It may be more clear to rename some or all of the with functions to have more descriptive names.

Oh man, that makes a ton of sense! Goes totally in line with the point I made above about converting values.

For what it's worth, I've already tried to avoid function names like with where a function might conceivably made part of a pipeline. For example, there used to be

Frame3d.at : Point3d -> Frame3d

to conveniently create a Frame3d with a given origin point but the same orientation as the global frame. This read OK in conjunction with an argument (Frame3d.at somePoint), but then I realized that it didn't work well in pipelines (somePoint |> Frame3d.at) so I renamed it to Frame3d.atPoint. For all of the with functions I couldn't see a realistic way to use them in pipelines (even with some partial application) so it seemed more OK to have them only read well in conjunction with their arguments.

[tomek-he-him]

For what it's worth, I've already tried to avoid function names like with where a function might conceivably made part of a pipeline.

Yeah, the lighter weight of the API is one thing, pipeline friendliness is another that I thought speaks towards conventional function arguments and against records. The other thing I was thinking of was pipeline-friendliness. 😄

But you’re totally right – while using OpenSolid I can’t remember bumping into a situation where I was building a pipeline and stumbled upon a with instead of something pipeline friendly. You did a great job there man!

 

There are also a couple cases already that use a hybrid approach, like Arc3d.around which takes an Axis3d as the first argument but then a record for the remaining arguments. The idea was that the first argument is obvious from the function name, and it also formats pretty nicely with elm-format

I like the look of it! Judging by the list of with functions that would be renamed though, the need for a record passed as the last argument would be an exception rather than a rule, wouldn’t it?

 

By the way, of course my sample had a typo in there. It read

myAxis
    |> Axis2d.direction
    |> Direction2d.angle

while what I meant is of course

    |> Direction2d.toAngle

Corrected already, apologies for any confusion!

[tomek-he-him]

The module renaming question might be one to open up to the broader community via a post to the 'Request Feedback' topic on the Elm Discourse.

Yeah, good idea. I don’t have enough knowledge to be more helpful than pointing out that renaming every module on export feels a bit “non-native” to me. I only know the Elm module system as a consumer of packages – don’t know the internals of how they work, how conflicts are resolved etc. I also never used Haskell seriously, and it does seem like Elm’s handling of imports is modeled after Haskell’s.

One observation that might be worth bringing into the discussion: despite the general community push towards generic, descriptive, top-level modules, elm-community/linear-algebra exports everything as second level modules grouped under the Math. namespace.

[ianmackenzie]

Corrected already, apologies for any confusion!

No worries, I think I just interpreted it as an example of the current naming looking weird =)

Judging by the list of with functions that would be renamed though, the need for a record passed as the last argument would be an exception rather than a rule, wouldn’t it?

Yeah, probably - might just be the ellipse/elliptical arc constructors. I think there would be a couple cases that would stay as a single record argument even after a rename, though - e.g. without a record I don't see any good way to make it clear in what order you would pass the four arguments to BoundingBox2d.fromExtrema!

I've posted a topic asking about the module prefixes: https://discourse.elm-lang.org/t/removing-prefixes-from-module-names/523

[nmsmith]

Regarding module names, why not make all the modules a sub-module of "Geometry"? So have Geometry.Point2d, Geometry.Scalar etc.

After all, this is a geometry package ;)

[ianmackenzie]

I actually am using a version of that right now =) I think that Geometry.Point2d is a bit redundant (what would a non-geometric Point2d be?) but I'm planning to have a few modules like Geometry.Accuracy where a non-prefixed name would be more ambiguous.

For Scalar and Float, those have now been split out into their own separate, standalone packages - ianmackenzie/elm-interval with an Interval module and ianmackenzie/elm-float-extra with a Float.Extra module.

[ianmackenzie]

Hey all - 3.0 a.k.a. elm-geometry 1.0 is almost ready for release, with draft release notes here. Would love to get your thoughts on the various changes!

[ianmackenzie]

Closed now that 1.0 has been released - relevant bits moved to #56.