Skip to content

Conversation

@peterstace
Copy link
Owner

Description

The vast majority of constructors are now in the form NewFoo, where Foo is the name of the thing being constructed (e.g. a geometry type such as MultiPolygon).

Previously, the constructors were named so that the type that was being constructed from was also mentioned.

E.g. NewPolygonFromRings was renamed to NewPolygon. The FromRings part really was redundant, since polygons are always constructed from their constituent rings (there really is no other practical way to construct them).

Check List

Have you:

  • Added unit tests? N/A, just a rename.

  • Add cmprefimpl tests? (if appropriate?) N/A.

Related Issue

N/A

Benchmark Results

N/A

The vast majority of constructors are now in the form `NewFoo`, where
`Foo` is the name of the thing being constructed (e.g. a geometry type
such as `MultiPolygon`).

Previously, the constructors were named so that the type that was being
constructed _from_ was also mentioned.

E.g. `NewPolygonFromRings` was renamed to `NewPolygon`. The `FromRings`
part really was redundent, since polygons are always constructed from
their constituent rings (there really is no other practical way to
construct them).
@peterstace peterstace self-assigned this Aug 30, 2021
Copy link
Collaborator

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

I like it 👌

@peterstace
Copy link
Owner Author

peterstace commented Aug 30, 2021

Screen Shot 2021-08-31 at 5 55 16 am

The build is failing due to a 504 from coveralls... The actual main CI part is working though. I'm going to ignore it for now, and investigate what's going wrong there if it keeps happening (I'm guessing it's probably a transient problem).

@peterstace
Copy link
Owner Author

Thanks for the speedy review @albertteoh 🥳

@peterstace peterstace merged commit ad4a92d into master Aug 30, 2021
@peterstace peterstace deleted the rename_geometry_constructor_methods_for_consistency branch August 30, 2021 19:56
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