Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
GH-33526: [R] Implement new function open_dataset_csv with signature more closely matching read_csv_arrow #33614
GH-33526: [R] Implement new function open_dataset_csv with signature more closely matching read_csv_arrow #33614
Changes from all commits
f7c63cf
c7578ac
3e932c0
e057b81
cb2b365
3cbf7a8
cc16968
df25d61
5aa4525
aeb1dbf
1528b95
63f223f
fa50ef0
72f2e7e
677ca8e
e86d84f
d8ae2c9
cf95f94
7f858ea
983362e
40199c0
761afd3
26f9f1b
fb087a9
fdc2658
059fe5e
0468160
7d6b1ce
c85808a
54b529d
785db46
29da6a8
b710eb8
3388076
b3c27a0
e0e8411
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whereby:
...
it gets harder for users to figure out what they can specify there (and specifically here you'll have to dig through more levels of source to figure it out),suggested:
?FileFormat
a bit more?CsvFileFormat
exists if this is going to@rdname
there?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points made; the refactoring I did here is because that bit of the code was overly complex, so in the name of usability, I think it'll make more sense to write actual independent docs for this function with more explicit information about its usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if
args[["parse_options"]]
is not a list? (Probably should be in the comments since I can't tell by reading this what it should or should not be)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
args[["parse_options"]]
can either be a list of options, or aCsvParseOptions
object. This helper function is only designed to validate the former; the latter is more advanced/off-label usage and it's up to users to safely pass the correct arguments through here (you could argue that this shouldn't be the case, but doing it this way is consistent with how we've done this in other bits of the codebase).I thought about documenting this, but I don't know where it would go - I don't think inside this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could redocument
#' @param parse_options
inopen_delim_dataset()
? (Or add to the existing#' @param parse_options
if the behaviour is the same for bothopen_delim_dataset()
andread_delim_arrow()
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's off-label usage then you probably should error for it...with the new function we have the opportunity to protect users from off-label usage (which was very very easy to do with the existing
open_dataset()
). Passingparse_options = CsvParseOptions$create(...)
was the first thing I thought to do and so I imagine somebody else might think to do it, too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parse_options = CsvParseOptions$create(...)
is the way that users have been doing things thus far as passing in lists is new - this additional way of doing things only got merged in in the past few weeks. I'm not sure what the right call is here; I don't want to error on passing in aCsvParseOptions
object and break folks' existing code but do want to encourage passing in options as a list so it actually hits our validation code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see...let's consider that later, then, but make it clear in this function that those are the two options for future us trying to read this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added more info here, and more explicit acknowledgement of this to
CsvFileFormat$create()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is all existing code which I just extracted out into its own function