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 argument include_file_paths in pl_scan_csv #1238

Merged
merged 6 commits into from
Sep 23, 2024

Conversation

collioud
Copy link
Contributor

@collioud collioud commented Sep 22, 2024

Context
The argument include_file_paths is missing from the pl_scan_csv and pl_read_csv functions whereas it is available for other pl_read_... fonctions (for example, pl_read_parquet).
Close #1235

Goal of this PR
Add the argument include_file_paths to the pl_scan_csv and pl_read_csv functions.
Adjust the related documentation.

Tests
Not tested yet

Hope this will help the project!
Cheers

Copy link
Collaborator

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me, there are just a couple of things to fix in tests.

Also, can you add a bullet point in NEWS.md indicating the new feature and the PR number (and your github username if you want)?

tests/testthat/test-csv-read.R Outdated Show resolved Hide resolved
tests/testthat/test-csv-read.R Show resolved Hide resolved
@collioud
Copy link
Contributor Author

Made a commit with the asked modifications.
Have a good night!

@etiennebacher
Copy link
Collaborator

Almost done, you just need to redocument. You can use task build-documents for that

@collioud
Copy link
Contributor Author

Done.
Please note that I changed a bit the tests/testthat/test-csv-read.R file to be coherent with test-parquet.R.
When doing task build-documents, I also have modifications in some doc files (like man/Series_to_r.Rd, man/Series_class.Rd, etc.), but it looks like it is unrelated to this PR, as far as I know. I let the changes, but feel free to discard them...

@etiennebacher
Copy link
Collaborator

I cleaned up the PR, I think the extra changes in .Rd files were due to the fact you don't have the latest version of roxygen2.

That's a nice first contribution, thanks @collioud!

@collioud
Copy link
Contributor Author

You made this easy!
Thanks again for such a nice and useful project!

@etiennebacher etiennebacher merged commit 0412883 into pola-rs:main Sep 23, 2024
33 of 35 checks passed
@collioud collioud deleted the add_include_file_paths branch September 23, 2024 12:41
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.

Add the include_file_paths parameter for all read_... functions?
2 participants