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

feat(python): Allow mapping as syntactic sugar in str.replace_many #18214

Merged
merged 10 commits into from
Aug 27, 2024

Conversation

henryharbeck
Copy link
Contributor

@henryharbeck henryharbeck commented Aug 15, 2024

Closes #17220

Functionality and implementation is most similar to Expr.replace, but avoids the issue raised in #18185

Also makes a few other minor changes in support:

As a heads up, this feature does now result in a different error message (but the same type) when replace_many is called with a single (non-mapping) argument.

df = pl.DataFrame({"a": ["a", "b", "c"]})
df.select(pl.col("a").str.replace_many(["b"]))
# Before: TypeError: ExprStringNameSpace.replace_many() missing 1 required positional argument: 'replace_with'
# After:  TypeError: `replace_with` argument is required if `patterns` argument is not a Mapping type

@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature python Related to Python Polars labels Aug 15, 2024
Comment on lines -6826 to -6829
Returns
-------
DataFrame

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drive by: Returns section is not consistent with other DataFrame methods and LazyFrame.join docstring. Is also already covered by type hint.


This version determines if any of the patterns find a match.
Determines if any of the patterns are contained in the string.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not clear what is being compared against "this version". Also updated to be a bit more explicit.

)


def test_replace_many_invalid_inputs() -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could alternatively be moved to py-polars/tests/unit/test_errors.py if so desired.

_reserved_namespaces: set[str] = reduce(
or_,
(cls._accessors for cls in (pl.DataFrame, pl.Expr, pl.LazyFrame, pl.Series)),
_reserved_namespaces: set[str] = set.union(
*(cls._accessors for cls in (pl.DataFrame, pl.Expr, pl.LazyFrame, pl.Series))
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drive by: intent is clearer IMO and removes the need for two imports

@henryharbeck henryharbeck marked this pull request as draft August 15, 2024 14:32
@henryharbeck

This comment was marked as resolved.

@henryharbeck henryharbeck marked this pull request as ready for review August 15, 2024 15:22
Copy link

codecov bot commented Aug 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.84%. Comparing base (4157e92) to head (a46f63f).
Report is 83 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #18214      +/-   ##
==========================================
- Coverage   80.30%   79.84%   -0.47%     
==========================================
  Files        1498     1496       -2     
  Lines      198751   200345    +1594     
  Branches     2833     2844      +11     
==========================================
+ Hits       159598   159956     +358     
- Misses      38626    39864    +1238     
+ Partials      527      525       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@henryharbeck
Copy link
Contributor Author

Hi @ritchie46 @alexander-beedie @MarcoGorelli
May I please request a review on this PR

Cheers

@ritchie46 ritchie46 merged commit c579721 into pola-rs:main Aug 27, 2024
15 checks passed
@henryharbeck henryharbeck deleted the replace branch August 27, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or an improvement of an existing feature python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

str.replace_many to take a dictionary that defines a replacement mapping.
2 participants