-
Notifications
You must be signed in to change notification settings - Fork 56
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 R examples and geoarrow #108
Conversation
Awesome! Excited to add this. It looks like you had a markdown formatter going so this PR has a lot more changes than just the couple of new lines. Could you make it so it's just a PR on the new lines? I don't think we're opposed to markdown formatting improvements, but it'd be best to put them in their own, clean PR so the changes are clear. I do think we should move towards some automated formatting or at least lint checking for markdown, so it is all consistent. |
probably good to cc @paleolimbot directly too for 👍 |
The example looks great! Thank you for adding! I agree about the Markdown formatting...it's considered bad git karma but when this happens to me I usually just copy/paste the original back from GitHub, find my changes from the Changed Files tab on the PR, and copy/paste those back into the document (with apologies if you already know all about this!). |
Thanks for the tips, actually one of my first PR's so appreciate it. Have some automatic linting going on on save. I hope this last commit fixes it ? |
Sorry for the delay on this, but looks great - thanks for your contribution @yeelauren! |
Ugh, I merged this but it looks like it lead to a lint failure - I apologize. I have to run right now, but I'll try to investigate soon. It'd be good to run our linter on branches and not merge if it's failing, so we hit the errors before merging instead of after. |
Lint is already set up to run on every pull request: geoparquet/.github/workflows/lint.yml Line 7 in d00ad62
On the first commit on this PR lint failed, but then not sure why no later commits show that CI was run at all |
This also makes me wonder: do we somehow want to keep track for each implementation we list which version they support? |
Added
geoarrow
package for R in readme and a minimal example with plotting.