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

Align arguments of several string to date(time) functions with python polars #939

Merged
merged 8 commits into from
Mar 21, 2024

Conversation

etiennebacher
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Thanks, but honestly, I feel that checking dots is overkill here.
(If we do it, wouldn't it be better to import rlang and use check_dots_empty etc to simplify things?)

R/expr__string.R Outdated Show resolved Hide resolved
R/expr__string.R Outdated Show resolved Hide resolved
R/expr__string.R Outdated Show resolved Hide resolved
R/expr__string.R Outdated Show resolved Hide resolved
R/expr__string.R Outdated Show resolved Hide resolved
R/expr__string.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Once again, on second thought, I disagree with the idea of doing a dots check process for variable name changes.
There are currently so many variable mismatches with Rust/Python Polars in this package that I don't want to take on the huge amount of work involved in writing and later removing all those checks.

It would be better to get the variable name fixes done as quickly as possible than to expend the effort on such a thing.

@etiennebacher
Copy link
Collaborator Author

etiennebacher commented Mar 19, 2024

There are currently so many variable mismatches with Rust/Python Polars in this package that I don't want to take on the huge amount of work involved in writing and later removing all those checks.

True but then I think we should advertise more in the README that the package is unstable and that naming all args is a good practice to reduce breaking changes.

I'll remove those checks

@eitsupi
Copy link
Collaborator

eitsupi commented Mar 19, 2024

we should advertise more in the README that the package is unstable and that naming all args is a good practice to reduce breaking changes.

I think that makes sense.
(Although I personally believe that breaking changes can happen at any time for packages that follow semantic versioning.)

Copy link
Collaborator

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Thanks.

One minor comment.

README.md Outdated Show resolved Hide resolved
@eitsupi eitsupi merged commit 115832c into main Mar 21, 2024
19 checks passed
@eitsupi eitsupi deleted the rename-str-time-args branch March 21, 2024 12:01
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.

None yet

2 participants