Skip to content

ARROW-6337: [R] Changed as_tible to as_dataframe in the R package - #5399

Closed
jameslamb wants to merge 2 commits into
apache:masterfrom
jameslamb:as_dataframe
Closed

ARROW-6337: [R] Changed as_tible to as_dataframe in the R package#5399
jameslamb wants to merge 2 commits into
apache:masterfrom
jameslamb:as_dataframe

Conversation

@jameslamb

@jameslambjameslamb commented Sep 17, 2019

Copy link
Copy Markdown
Contributor

See ARROW-6337 for background on this PR.

@codecov-io

codecov-io commented Sep 17, 2019

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@6d4f257). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@## master #5399 +/- ##
=========================================
Coverage ? 76.09% =========================================
Files ? 56 Lines ? 3572 Branches ? 0 =========================================
Hits ? 2718 Misses ? 854 Partials ? 0
Impacted FilesCoverage Δ
r/R/feather.R63.33% <100%> (ø)
r/R/csv.R100% <100%> (ø)
r/R/json.R100% <100%> (ø)
r/R/parquet.R66.66% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d4f257...66137bb. Read the comment docs.

@nealrichardsonnealrichardson left a comment

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.

Acknowledging that naming things is hard, I'm not a fan of as_dataframe. That doesn't look right to me. I'd prefer as_data_frame, or even data_frame is better IMO--I'm not sure "as" adds any value.

What do you think?

@jameslamb

Copy link
Copy Markdown
ContributorAuthor

Acknowledging that naming things is hard, I'm not a fan of as_dataframe. That doesn't look right to me. I'd prefer as_data_frame, or even data_frame is better IMO--I'm not sure "as" adds any value.

What do you think?

IMO as_data_frame or return_data_frame is more informative than data_frame, so it's obvious in code that it isn't, for example, a flag used to allow you to pass in parse_options as a data frame instead of named list or something.

As long as we're keeping the "change output type with a flag" pattern, my preferences in order are:

  1. as_data_frame
  2. return_data_frame
  3. data_frame

I hold that opinion lightly though, and I'll defer to whatever you think is best on this one. Let me know what you'd like to do and I'd be happy to update the PR today!

@nealrichardson

Copy link
Copy Markdown
Member

Alright, then as_data_frame seems to be the compromise solution. Let's go with that.

@jameslamb

Copy link
Copy Markdown
ContributorAuthor

Alright, then as_data_frame seems to be the compromise solution. Let's go with that.

🤝 updated to as_data_frame and rebased to most recent master

@nealrichardson

Copy link
Copy Markdown
Member

+1, thanks!

kou pushed a commit that referenced this pull request Feb 20, 2023
Sign up for freeto 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.

3 participants

@jameslamb@codecov-io@nealrichardson