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 support for GeoJSON #123

Merged
merged 4 commits into from
Jul 19, 2019
Merged

Add support for GeoJSON #123

merged 4 commits into from
Jul 19, 2019

Conversation

sharlagelfand
Copy link
Contributor

Adding support for GeoJSON! I think I covered everything (added an example, added geojsonio to suggests, reran codemeta, ran docs, etc). But let me know if I missed anything.

Also, thank you for adding me as an author! ☺️

@sckott sckott modified the milestones: v0.4, v0.3 Jul 18, 2019
Copy link
Contributor

@sckott sckott left a comment

Choose a reason for hiding this comment

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

thanks @sharlagelfand !

I like that you used geojsonio for obvious reasons, but I wonder since sf is already used here, if we just use sf instead? I guess it depends on what kind of data you'd want back. Thoughts?

R/ckan_fetch.R Outdated Show resolved Hide resolved
@sharlagelfand
Copy link
Contributor Author

honestly i don't know much about GeoJSON and just googled packages for it -- used geojsonio for the obvious reason without realizing that sf could handle GeoJSON too! i agree, since sf is already used it makes sense to use that too. i'll update the code.

ps, i'm working on this package hence my sudden ckanr investment 😂 thank you for being so receptive to these changes!

@sharlagelfand
Copy link
Contributor Author

sorry for the messy commits 🙈 rebasing is hard

@sckott
Copy link
Contributor

sckott commented Jul 18, 2019

thanks, looks good. cool about the package (see also #113 and https://github.com/VLucet/rgovcan - also canada)

just one change, can you undo the codemeta.json file cahange, so we can keep the PR focused on the actual changes here

@sharlagelfand
Copy link
Contributor Author

done, thank you! thanks for linking that package too!

@sckott
Copy link
Contributor

sckott commented Jul 19, 2019

thanks @sharlagelfand

@sckott sckott merged commit 02889d7 into ropensci:master Jul 19, 2019
@sharlagelfand sharlagelfand deleted the geojson branch August 8, 2019 22:17
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.

2 participants