-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Discussion: Changing the Signatur of the Function #9
Comments
@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 If you agree with my opinion, we can keep Maybe we can provide reasonable defaults and make the |
We've also had a couple of issues open because people pass 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: |
This sound totally reasonable, and I totally agree when you put it as above.
I was also also thinking about this. Sounds like a good suggestion to allow both I will raise issues for this later (unless you bet me to it :) ). |
After thinking, I think I will add the following issues and then start solving them:
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? |
I started a version 2.1.0 project. Hope that is ok :) Feel free to change or remove it if you disagree. |
I just discovered that the function accepts arrays with a single value as a argument for the parameter Technically, stopping accepting arrays as input for My question is:
|
@johachi I've been nonresponsive because my schedule has been really complicated and I have some family situations I need to deal with.
I prefer to delay it until
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). |
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.
Like a disclaimer in the readme, or as a console.warn? Also, I hope your family situation is getting better! |
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 |
Sounds good to me. Will do so tomorrow.
Thank you very much. I will add this tomorrow when I add the disclaimer about floats. :D |
Done in PR #29 |
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
Solving the problem with too large positive or negative
lat
andlng
values has to be solved by creating aMultiPolygon
instead of the regularPolygon
. 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 eithernumberOfSegments
or bothradius
andnumberOfSegemnts
with an object calledconfig
.The new arguments would then be just
center
andconfig
and the function call would becircleToPolygon(center, config)
where config could look something like this: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
The text was updated successfully, but these errors were encountered: