Skip to content
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

added test cases for issue #852 #892

Merged
merged 6 commits into from
Aug 11, 2017
Merged

Conversation

grundid
Copy link
Contributor

@grundid grundid commented Aug 7, 2017

The tests fail as expected.

@DenisCarriere
Copy link
Member

DenisCarriere commented Aug 7, 2017

Issue: #852

precision start point
image

@stebogit
Copy link
Collaborator

stebogit commented Aug 7, 2017

Ooops I just wanted to add my changes to @grundid's ones, but that (of course! 😞 ) actually merged his branch to mine. Sorry, I guess I should have merged with --no-ff...
This PR is still active, but the branch it refers doesn't exist anymore. Sorry again.
@DenisCarriere how can I fix this?

Anyway, I noticed increasing the precision of truncate to 7 or 8 (still above GeoJSON recommendations) solves 3 of the 4 new tests. The one still failing is the precision end point

@DenisCarriere
Copy link
Member

I would pull down his branch and make the changes from there, don't know of an easy way to do that, it's only 2 files so I would end up doing it manually.

@DenisCarriere
Copy link
Member

Not too sure if changing the decimal points of truncate would be the solution here, I think better handling of the exact start & end points would be a better approach.

Seems like it's having difficulties with exact coordinates, not too sure if we can exclude points from generating lines which are less then 10cm (0.000001 degrees) apart.

@DenisCarriere
Copy link
Member

@stebogit That's what happens when you reply to GitHub issues on the bus 😆

@DenisCarriere
Copy link
Member

@stebogit Add your commit to this branch

@stebogit
Copy link
Collaborator

stebogit commented Aug 9, 2017

Thanks @DenisCarriere to fixing my mess!
🤔 hmmm... so I need to clone geohub-plattform repo and commit to master, right?

@DenisCarriere
Copy link
Member

That's correct, ideally we would create a branch on TurfJS, that way it's a bit easier to switch between branches.

@stebogit
Copy link
Collaborator

Ok, it should be fixed now.
@grundid could you please check this?

Copy link
Member

@DenisCarriere DenisCarriere left a comment

Choose a reason for hiding this comment

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

Sweet! 👍 Looks good to me

@DenisCarriere DenisCarriere merged commit ad69daa into Turfjs:master Aug 11, 2017
DenisCarriere added a commit that referenced this pull request Aug 11, 2017
DenisCarriere added a commit that referenced this pull request Aug 11, 2017
@DenisCarriere
Copy link
Member

@grundid Just published a patch release for @turf/line-split@4.6.1 with these fixes. That way it's a bit easier to test on your side.

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