search

ateucher / rmapshaper

An R wrapper for the mapshaper javascript library
http://andyteucher.ca/rmapshaper/
Other
200 stars 13 forks source link

Fixunits #116

Closed Robinlovelace closed 2 years ago

Robinlovelace commented 2 years ago

Nudge on this @ateucher if you get a chance please take a look.

ateucher commented 2 years ago

Thanks @Robinlovelace - and I actually just realized that it would be great to pull this chunk out into a small function of its own so it can be used in the other ms_* functions... and also easier to test. I'm happy to do it if you don't have the time.

Robinlovelace commented 2 years ago

Hitting some issues with your suggested changes, thanks for the offer @ateucher I'll reply in ~5 let's see where I get to and v. grateful if you can finish the job. One more commit incoming...

Robinlovelace commented 2 years ago

OK my PR is done, do with it what you will. Note the for loop is I think clearer than the lapply stuff which seemed to have side effects. Also I think [[ is better than [ here because sf objects are sticky. Many ways of doing these things (the joys of R ... ; ) and happy with anything that works, 3 year issue... nearly fixed, many thanks!

ateucher commented 2 years ago

Thanks again! Right, good call on working around the sticky sf column, I hadn't thought of that. And the for loop is just fine!

Robinlovelace commented 2 years ago

Never say final. Didn't save changes to test. See latest commit @ateucher in case you've already just started on this.

ateucher commented 2 years ago

Looking great! Just waiting for the checks to complete - but do you mind removing the new example you added? I don't think it's necessary to test this functionality in the examples, and I'd rather not add spData to Suggests - which would be best practice if using it an an example.

ateucher commented 2 years ago

Thanks again @Robinlovelace! Great to have this finally done :) The CI failures are not due to this PR, so I'll merge it now!