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

Add snap interval #43

Merged
merged 1 commit into from
Sep 2, 2016
Merged

Add snap interval #43

merged 1 commit into from
Sep 2, 2016

Conversation

nikolai-b
Copy link
Contributor

Also looks like drop_null_geometries wasn't being pass through for geo_list so I've added that in this PR.

@ateucher
Copy link
Owner

Thanks @nikolai-b! I'll probably have time to review this in a week or so, but if you want to check into why the builds are failing that would be awesome. (#23)

@nikolai-b
Copy link
Contributor Author

Cheers, will do

@codecov-io
Copy link

codecov-io commented Aug 26, 2016

Current coverage is 98.63% (diff: 88.88%)

Merging #43 into master will decrease coverage by 0.52%

@@             master        #43   diff @@
==========================================
  Files            12         12          
  Lines           357        367    +10   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            354        362     +8   
- Misses            3          5     +2   
  Partials          0          0          

Powered by Codecov. Last update 6b0a043...f765f08

@nikolai-b
Copy link
Contributor Author

Ok, updated. I removed
3ed81c9
from this pull as that seem to break the specs. I'm not sure why as it looks like simplify is ignoring dropping null geometries, is that intentional? The docs seem to mention it is always true for spatial objects but not that it is always FALSE for geo_lists. Sorry if I missing something obvious.

@@ -173,12 +189,13 @@ make_simplify_call <- function(keep, method, keep_shapes, no_repair, snap, explo
}

if (explode) explode <- "-explode" else explode <- NULL
if (snap && !is.null(snap_interval)) snap_interval <- paste("snap-interval=", snap_interval)
Copy link
Owner

Choose a reason for hiding this comment

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

This should be paste0 instead of paste, but I'll make the fix after I merge it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eeek, of course, thanks for spotting.

@ateucher ateucher merged commit 2071eee into ateucher:master Sep 2, 2016
@ateucher
Copy link
Owner

ateucher commented Sep 2, 2016

Sorry for taking so long to get to this @nikolai-b - it looks good! There's one small fix that needs to be made, (see inline comments), but I'll do it after I merge. Then I'll add a few tests.

Thanks so much for the contribution!

As for the dropping null geometries, I went in a circles a bit on that, so I'm not overly surprised it's inconsistent. I'll have a look - would you mind opening a separate issue to remind me?

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