Skip to content

ARROW-5500: [R] read_csv_arrow() signature should match readr::read_csv()#4711

Closed
nealrichardson wants to merge 5 commits into
apache:masterfrom
nealrichardson:readr-csv
Closed

ARROW-5500: [R] read_csv_arrow() signature should match readr::read_csv()#4711
nealrichardson wants to merge 5 commits into
apache:masterfrom
nealrichardson:readr-csv

Conversation

@nealrichardson
Copy link
Copy Markdown
Member

This patch enumerates the various CSV parsing options and exposes them in an R-familiar way in the signature of read_csv_arrow(). It also adds a generic read_delim_arrow() for providing other delimiting characters, as well as a read_tsv_arrow(). In the process, I identified some limitations of the current reader (https://issues.apache.org/jira/browse/ARROW-5747) and of the R bindings to it (not yet ticketed), and added more docs and tests.

Other release-prep cleanup in here includes organization of the DESCRIPTION file, adding new functions to the pkgdown config, and adding a NEWS.md.

Copy link
Copy Markdown
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

Haven't looked carefully at the R code (could @romainfrancois take a look)? But I found the version number in NEWS.md to be misleading, since a CRAN release probably won't happen until after 0.14.0 goes out

Comment thread r/NEWS.md
under the License.
-->

# arrow 0.13.0.9000
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It should. https://issues.apache.org/jira/browse/ARROW-5415 will update it to the release version in dev/release/00-prepare.sh. I have a branch for that based on this one.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

+1, modulo CI. Taking a look

@wesm
Copy link
Copy Markdown
Member

wesm commented Jun 27, 2019

Here's a passing Travis build: https://travis-ci.org/nealrichardson/arrow/builds/551482949

Appveyor running here: https://ci.appveyor.com/project/nealrichardson/arrow/builds/25596580. Will wait a bit yet

@wesm
Copy link
Copy Markdown
Member

wesm commented Jun 27, 2019

The R Appveyor build is running now so I'll merge once that passes

@wesm
Copy link
Copy Markdown
Member

wesm commented Jun 27, 2019

Build is passed

@wesm wesm closed this in d8b3be9 Jun 27, 2019
kou pushed a commit that referenced this pull request Jun 28, 2019
In previous patches, I removed other places where the version was explicitly referenced, making them parse it from `r/DESCRIPTION` where necessary. The only remaining one (added by #4711) is `r/NEWS.md`, and it works a little differently when bumping from a release version to a dev version. So from dev to release, we go from e.g.

```
# arrow 0.13.0.9000

* Stuff we did in this release
* More stuff
```

to

```
# arrow 0.14.0

* Stuff we did in this release
* More stuff
```

but when bumping after release for the next dev version, it goes to

```
# arrow 0.14.0.9000

# arrow 0.14.0

* Stuff we did in this release
* More stuff
```

so that we have a place to accumulate the list of changes in the next release.

Author: Neal Richardson <neal.p.richardson@gmail.com>
Author: Sutou Kouhei <kou@clear-code.com>

Closes #4727 from nealrichardson/r-version-update and squashes the following commits:

f3dfa66 <Sutou Kouhei> Use  for consistency
1ae324f <Neal Richardson> Review feedback
090e3a5 <Neal Richardson> Update r/NEWS.md with release or new dev version
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