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 arc_read() (#108) #118

Merged
merged 12 commits into from
Dec 27, 2023
Merged

Add arc_read() (#108) #118

merged 12 commits into from
Dec 27, 2023

Conversation

elipousson
Copy link
Contributor

Checklist

  • update NEWS.md
  • documentation updated with devtools::document()
  • devtools::check() passes locally (with the changes on main)

Changes

Per #108, I added a arc_read() function that works as a convenient alternative to arc_open() |> arc_select() and adds a col_names and name_repair parameter (an idea discussed briefly at #60 ) to provide a similar interface to readr::read_delim() and other tidyverse read functions.

The function documentation includes examples and the PR includes tests.

Follow up tasks

After this is added, it should likely be referenced in the README. If/when a getting started vigenette is added, it could also be documented there.

@JosiahParry
Copy link
Collaborator

holy smokes! Missed this! Will review

@JosiahParry
Copy link
Collaborator

This PR has conflicts with the man page of obj_check_layer and NEWS.md. Can you resolve the issues? Preferably this PR would have only modified the NAMESPACE and DESCRIPTION and created new files.

@elipousson
Copy link
Contributor Author

@JosiahParry Happy to fix the conflicts. I was having trouble figuring out how to see where the conflicts were located. I need to get just a little bit better at Git + GitHub so I can figure out what to when I've already started on a PR and missed updates on the main repo. Probably should be using feature branches!

If I should leave NEWS.md alone in the future, do you want to take it out of the PR checklist? Or is the intent for the checklist to be completed during review?

@JosiahParry
Copy link
Collaborator

Ah, yah, no, NEWS is totally fine to update too! The big conflicts come from documenting functions from outside of the scope of the PR.

If you did all of this on main you may want to checkout a new branch with your code changes. Copy them somewhere else, then on main run git reset --hard to get the status of the main branch. Then, copy the relevant changed files onto a new branch.

May be easiest to make a new PR instead of doing merge conflicts lol

@elipousson
Copy link
Contributor Author

No rush to review, @JosiahParry, but I did want to confirm that I think the merge conflicts are resolved.

@JosiahParry
Copy link
Collaborator

Thank you for the reminder. Yes, merge conflicts are resolved!

Copy link
Collaborator

@JosiahParry JosiahParry left a comment

Choose a reason for hiding this comment

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

Sorry it took so long for me to get to this!! Thank you for keeping me honest here. I think its mostly ready! Just need some doc changes and minor things

R/arc-read.R Outdated Show resolved Hide resolved
R/arc-read.R Outdated Show resolved Hide resolved
R/arc-read.R Outdated Show resolved Hide resolved
R/arc-read.R Outdated Show resolved Hide resolved
R/arc-read.R Outdated Show resolved Hide resolved
R/arc-read.R Outdated Show resolved Hide resolved
R/arc-read.R Outdated Show resolved Hide resolved
R/arc-read.R Outdated Show resolved Hide resolved
R/arc-read.R Outdated Show resolved Hide resolved
R/arc-read.R Outdated Show resolved Hide resolved
Also remove warning if n_max specifies fewer features than the max available
Copy link
Collaborator

@JosiahParry JosiahParry left a comment

Choose a reason for hiding this comment

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

Merry chirstmas 🎄🎅🏼 Thank you!

I think the last change we need here is to get rid of the field argument. It seems that col_select and fields are functionally identical.

R/arc-read.R Outdated Show resolved Hide resolved
R/arc-read.R Outdated Show resolved Hide resolved
@JosiahParry
Copy link
Collaborator

I went and took the liberty of making some minor changes. I added a few tests as well as ensured that the function was robust against the wrong types.

Gonna merge! If we dislike anything lets get it fixed in the next few weeks. Going to start working towards a CRAN release of arcgisutils next week.

@JosiahParry JosiahParry merged commit faa2a19 into R-ArcGIS:main Dec 27, 2023
8 of 11 checks passed
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