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

chore(python): Disable clippy lint "too many arguments" for py-polars #11616

Merged
merged 8 commits into from
Oct 10, 2023
Merged

chore(python): Disable clippy lint "too many arguments" for py-polars #11616

merged 8 commits into from
Oct 10, 2023

Conversation

svaningelgem
Copy link
Contributor

@svaningelgem svaningelgem commented Oct 9, 2023

Removing all the arguments that are across the code by lifting the threshold to 99.

@github-actions github-actions bot added internal An internal refactor or improvement rust Related to Rust Polars labels Oct 9, 2023
@svaningelgem svaningelgem marked this pull request as ready for review October 9, 2023 18:32
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.

I think it makes sense to disable this for py-polars, as we don't really choose the number of arguments - it's dictated by the Python side, and having many arguments isn't that much of a problem due to default values.

I wouldn't support turning this off for the Rust code base. We should probably take a look at these functions and refactor them, instead of turning off the lints globally.

@svaningelgem
Copy link
Contributor Author

svaningelgem commented Oct 9, 2023

Well, when I encounter some daunting task like this, especially if it's sprinkled just about everywhere in the code, I tend to disable the warnings.
Because this will never get refactored (never say never, but from a practical point of view: highly unlikely).
And then the question becomes: do I want to add these exclusions just about everywhere I fail to adhere to the 7 argument limit, or just ignore them globally?
Flip-side, it's a good reminder if you add some new code that you're likely could do better...

@svaningelgem svaningelgem changed the title chore(rust): Removing "too many arguments" that is sprinkled 107 times across the code base chore(rust): Removing "too many arguments" that is sprinkled 23 times across the code base Oct 9, 2023
@stinodego stinodego changed the title chore(rust): Removing "too many arguments" that is sprinkled 23 times across the code base chore(python): Removing "too many arguments" that is sprinkled 23 times across the code base Oct 10, 2023
@github-actions github-actions bot added the python Related to Python Polars label Oct 10, 2023
@stinodego stinodego changed the title chore(python): Removing "too many arguments" that is sprinkled 23 times across the code base chore(python): Disable clippy lint "too many arguments" for py-polars Oct 10, 2023
@stinodego stinodego added python Related to Python Polars and removed python Related to Python Polars rust Related to Rust Polars labels Oct 10, 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.

I disabled the lint properly in lib.rs. This is good to go!

@stinodego stinodego merged commit ebe4793 into pola-rs:main Oct 10, 2023
14 checks passed
@svaningelgem svaningelgem deleted the removing-too-many-arguments branch October 10, 2023 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants