Discussion: Changing the Signatur of the Function

[johachi]

I'm currently working on fixing the problem regarding out of range values (see #4 and #6 ). Out of range problem occur when the edge of the circle cross the longitude value 180 or when the circle include any of the polar circles.

When doing this I started to think about the functions signature, or more specific, it's arguments

circleToPolygon(center, radius, numberOfSegments)

Solving the problem with too large positive or negative lat and lng values has to be solved by creating a MultiPolygon instead of the regular Polygon. I realize that this would also change the output object as well.

However, some implementations prefer to large lat/lng values over MultiPolygon. Because of that, I wanted to make using MultiPolygon to be an option. At the same time, we don't want to keep on increasing arguments. One solution could be to replace either numberOfSegments or both radius and numberOfSegemnts with an object called config.

The new arguments would then be just center and config and the function call would be circleToPolygon(center, config) where config could look something like this:

// example is with default values
const config = {
  radius: 1500,
  numberOfSegments: 32,
  useRightHandRule: true,
  MultiPolygon: true
}

I could make the function backwards compatible by checking variable type and for a third argument, but maybe a version bump and having breaking changes in the readme would give more clean code. What do you think? @gabzim

[gabzim]

@johachi, I've been entertaining the idea of improving the function signature. I think we can go in that direction.

My one suggestion would be to keep the function as easy to use as possible and for that, I don't think radius should be a configuration param. When you're using this function, you really care about two essential values: center and radius. The numberOfSegments and any other option we add is more of an implementation configuration, but the first two are data, not config.

If you agree with my opinion, we can keep circleToPolygon(center, radius, options). Is that reasonable to you?

Maybe we can provide reasonable defaults and make the options arg optional? It can't get much easier to remember than circleToPolygon(center, radius)

[gabzim]

We've also had a couple of issues open because people pass lat and lon in the array (coords are backwards), so I was wondering if supporting an alternative syntax for the center arg (and checking for array or object) would help make the function more readable? something along the lines of circleToPolygon({lat, lon}, radius, options).

This is not absolutely necessary but I'm wondering if it would help people use function better without having to know that you need to pass in the array in this order: [lon, lat]. Totally open to suggestions here.

[johachi]

(...) I don't think radius should be a configuration param. When you're using this function, you really care about two essential values: center and radius. The numberOfSegments and any other option we add is more of an implementation configuration, but the first two are data, not config.

If you agree with my opinion, we can keep circleToPolygon(center, radius, options). Is that reasonable to you?

This sound totally reasonable, and I totally agree when you put it as above.

I was wondering if supporting an alternative syntax for the center arg (and checking for array or object) would help make the function more readable? something along the lines of circleToPolygon({lat, lon}, radius, options).

I was also also thinking about this. Sounds like a good suggestion to allow both { lat, lng } and [lon, lat] as coordinate argument.

I will raise issues for this later (unless you bet me to it :) ).

[johachi]

After thinking, I think I will add the following issues and then start solving them:

• Accept both array as [lon, lat] and object as { lat || latitude, lon || lng || longitude } as the argument for center
• Accept both number or an object as the option parameter. When passed as object, the object shall be { numberOfSegments }
• Add functionality to choose to not follow right-hand-rule. (when options passed as object)
• Add functionality to create MulitPolygon as needed (when options passed as object)

I'm still thinking about issue #10 . The specs seem to say that a coordinate MUST have at least 2 coordinates, and SHOULD NOT have more than 3 elements. In other words, maybe we should allow for more than 3 elements, even if we only use the first two. What do you think?

[johachi]

I started a version 2.1.0 project. Hope that is ok :) Feel free to change or remove it if you disagree.

[johachi]

@gabzim

I just discovered that the function accepts arrays with a single value as a argument for the parameter numberOfSegments, but passing [10] and 10 does not result in identical circles. In other words, circleToPolygon([16.226412, 58.556493], 138, [10]) and circleToPolygon([16.226412, 58.556493], 138, 10) both work and produce similar but not identical circles.

Technically, stopping accepting arrays as input for numberOfSegments would be a breaking change, but the documentation only specify that numberOfSegments can take a number, so no correct usage of the library would be affected of the change.

My question is:

• do you prefer to delay the check if numberOfSegments is not a number or object (except array) until a version 3.0.0 or should I put it in version 2.1.0?

• Also, right now decimal numbers is accepted as argument as well. They also create a circle, but the last segment will have a different length than the other segments. Do you want me to fix this (by rounding the number), or just leave it as-is.

[gabzim]

@johachi I've been nonresponsive because my schedule has been really complicated and I have some family situations I need to deal with.

do you prefer to delay the check if numberOfSegments is not a number or object (except array) until a version 3.0.0 or should I put it in version 2.1.0?

I prefer to delay it until 3.0.0, you're correct that it would only hurt you if you're using the library incorrectly, but we're not sure who's relying that behavior so far so let's just defer.

Also, right now decimal numbers is accepted as argument as well. They also create a circle, but the last segment will have a different length than the other segments. Do you want me to fix this (by rounding the number), or just leave it as-is.

Ag, this is a good question, If the output produced by floating point numbers is incorrect, then we should add a disclaimer. If we round up/down internally, it can be obscure to a consumer of the function, I'd rather we put it on them to provide data that we can consume rather than making that decision for them (what if they always want to round down, or trunc, etc).

[johachi]

do you prefer to delay the check if numberOfSegments is not a number or object (except array) until a version 3.0.0 or should I put it in version 2.1.0?

I prefer to delay it until 3.0.0, you're correct that it would only hurt you if you're using the library incorrectly, but we're not sure who's relying that behavior so far so let's just defer.

I'm sorry, but it seems like I confused myself when looking at this.It was fixed in version 2.0.0 already. I jumped back too far when checking if it was possible to pass an array. It was possible in 1.X.X but not in 2.0.0.

Also, right now decimal numbers is accepted as argument as well. They also create a circle, but the last segment will have a different length than the other segments. Do you want me to fix this (by rounding the number), or just leave it as-is.

Ag, this is a good question, If the output produced by floating point numbers is incorrect, then we should add a disclaimer. If we round up/down internally, it can be obscure to a consumer of the function, I'd rather we put it on them to provide data that we can consume rather than making that decision for them (what if they always want to round down, or trunc, etc).

Like a disclaimer in the readme, or as a console.warn?

Also, I hope your family situation is getting better!

[gabzim]

Like a disclaimer in the readme

I figured a disclaimer in the readme would work. I don't think we should add a console.warn. If you update the readme, would you mind adding an Authors section and throwing your name if you want? I think the magnitude of your contributions and maintaining the project should be as visible as possible.

[johachi]

I figured a disclaimer in the readme would work. I don't think we should add a console.warn.

Sounds good to me. Will do so tomorrow.

If you update the readme, would you mind adding an Authors section and throwing your name if you want? I think the magnitude of your contributions and maintaining the project should be as visible as possible.

Thank you very much. I will add this tomorrow when I add the disclaimer about floats. :D

[johachi]

Done in PR #29