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

depr(python, rust!): Rename write_csv parameter quote to quote_char #11583

Merged

Conversation

svaningelgem
Copy link
Contributor

@svaningelgem svaningelgem commented Oct 7, 2023

closes #11545.

  • I left line_terminator for sink_csv/write_csv. And eol_char for read_csv/scan_csv. The reason is that for sink/write this isn't necessarily a character, but can be a string.

  • I adjusted the docs to explain this -hopefully- better for read/scan.

  • Adjusted delimiter -> separator, quote -> quote_char, comment -> comment_char where they were not the target.

@github-actions github-actions bot added internal An internal refactor or improvement python Related to Python Polars rust Related to Rust Polars labels Oct 7, 2023
@svaningelgem svaningelgem marked this pull request as ready for review October 7, 2023 14:28
@alexander-beedie
Copy link
Collaborator

So, aligning quote to quote_char is a winner - ensures both calls use the same param, makes perfect sense. You just need to make use of the @deprecate_renamed_parameter decorator to avoid making it a breaking change though.

However, I'd revert the change from separator to delimiter_char - I see why you're thinking about it, but it "feels" somewhat unwieldy, and we have already changed that param (from sep) in recent memory 🤔

@svaningelgem
Copy link
Contributor Author

svaningelgem commented Oct 8, 2023

However, I'd revert the change from separator to delimiter_char - I see why you're thinking about it, but it "feels" somewhat unwieldy, and we have already changed that param (from sep) in recent memory 🤔

I agree that it feels unwieldy, but I am in strong favor of consistency here.

fwiw: in CsvParserOptions, there is used delimiter, and then in PyBatchedCsv, there is used separator in the new method, which is passed to CsvReader in with_delimiter. So externally facing it's called -most of the time- separator, but internally it's most of the time delimiter...

I would strongly suggest to make up our minds ;-)

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Oct 8, 2023

I would strongly suggest to make up our minds ;-)

Yup; I'd suggest making it separator everywhere.
It is, after all, usually found "in the wild" as CSV, TSV, etc (not CDV, TDV, ... ;)

@svaningelgem
Copy link
Contributor Author

svaningelgem commented Oct 8, 2023

Yup; I'd suggest making it separator everywhere. It is, after all, usually found "in the wild" as CSV, TSV, etc (not CDV, TDV, ... ;)

Aaaargh, all that tedious work 😠 ... Let's see what I can easily revert.
Should I make it separator_char then? Or simply separator? I'd assume the latter.

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Oct 8, 2023

Yup; I'd suggest making it separator everywhere. It is, after all, usually found "in the wild" as CSV, TSV, etc (not CDV, TDV, ... ;)

Aaaargh, all that tedious work 😠 ... Let's see what I can easily revert. Should I make it separator_char then? Or simply separator? I'd assume the latter.

I think separator is the cleanest / least-awkward formulation 😅

Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

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

Thanks for this! A few comments - some can probably be done in follow-up PRs if you prefer.

py-polars/src/lazyframe.rs Outdated Show resolved Hide resolved
py-polars/src/lazyframe.rs Outdated Show resolved Hide resolved
py-polars/src/dataframe.rs Outdated Show resolved Hide resolved
py-polars/polars/lazyframe/frame.py Show resolved Hide resolved
py-polars/polars/dataframe/frame.py Outdated Show resolved Hide resolved
@svaningelgem svaningelgem requested a review from c-peters as a code owner October 8, 2023 20:21
@stinodego stinodego changed the title refactor: Align parameters in scan_csv/read_csv/sink_csv/write_csv. refactor(python, rust!): Align parameters in scan_csv/read_csv/sink_csv/write_csv. Oct 9, 2023
@github-actions github-actions bot added the breaking rust Change that breaks backwards compatibility for the Rust crate label Oct 9, 2023
Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

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

Great stuff, really appreciate the great attention to detail - that saves a lot of review cycles!

@stinodego stinodego changed the title refactor(python, rust!): Align parameters in scan_csv/read_csv/sink_csv/write_csv. depr(python, rust!): Rename write_csv parameter quote to quote_char Oct 9, 2023
@github-actions github-actions bot added the deprecation Add a deprecation warning to outdated functionality label Oct 9, 2023
@stinodego stinodego merged commit 3b3c4a0 into pola-rs:main Oct 9, 2023
31 checks passed
@svaningelgem svaningelgem deleted the fix_read_scan_write_csv_arguments branch October 9, 2023 05:01
@alexander-beedie
Copy link
Collaborator

Nice one :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking rust Change that breaks backwards compatibility for the Rust crate deprecation Add a deprecation warning to outdated functionality internal An internal refactor or improvement python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

read_csv vs write_csv: inconsistency between line_terminator and eol_char
3 participants