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: add more parameters to the fill_null method #1149

Conversation

IsaiasGutierrezCruz
Copy link
Contributor

What type of PR is this? (check all applicable)

  • πŸ’Ύ Refactor
  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ”§ Optimization
  • πŸ“ Documentation
  • βœ… Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below.

@github-actions github-actions bot added the enhancement New feature or request label Oct 7, 2024
Copy link
Member

@FBruzzesi FBruzzesi left a comment

Choose a reason for hiding this comment

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

This is great! Thank you some much for the effort!
I think the only bits missing to pass the ci are:

  • Testing the ValueError raise(s)
  • Silence the modin warning in pyproject.toml

Additionally I left a few comments in the code, leaning to the nitpick area πŸ˜‚

narwhals/_pandas_like/series.py Outdated Show resolved Hide resolved
narwhals/_pandas_like/series.py Show resolved Hide resolved
narwhals/_arrow/series.py Show resolved Hide resolved
@FBruzzesi
Copy link
Member

FBruzzesi commented Oct 9, 2024

I am tempted to say that coverage not showing may be an issue in pytest-cov itself - by rewriting the code with less nested statements, then it shows as covered πŸ€”

@IsaiasGutierrezCruz
Copy link
Contributor Author

I am tempted to say that coverage not showing may be an issue in pytest-cov itself - by rewriting the code with less nested statements, then it shows as covered πŸ€”

Yeah, it was a bit strange, but I've already refactored the code to pass the coverage tests. Thanks for pointing it out! c:

Copy link
Member

@FBruzzesi FBruzzesi left a comment

Choose a reason for hiding this comment

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

Thanks a ton @IsaiasGutierrezCruz ! I am really happy to see this one!

When I will be back from vacation I may try to replicate the issue with coverage as well πŸ™ƒ

BrownLabradorPuppyGIF

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

awesome work, thanks @IsaiasGutierrezCruz (and @FBruzzesi for reviewing!)

pyproject.toml Outdated Show resolved Hide resolved
narwhals/expr.py Outdated Show resolved Hide resolved
@IsaiasGutierrezCruz
Copy link
Contributor Author

I already resolved all the conversations πŸ˜ƒ @MarcoGorelli

@MarcoGorelli
Copy link
Member

thanks! there's a merge conflict, then i'll try to get this in

@MarcoGorelli MarcoGorelli reopened this Nov 2, 2024
tests/expr_and_series/fill_null_test.py Outdated Show resolved Hide resolved
tests/expr_and_series/fill_null_test.py Outdated Show resolved Hide resolved
@IsaiasGutierrezCruz
Copy link
Contributor Author

Hi @MarcoGorelli , I hope you're doing well! I’ve implemented the last suggestion you provided some time ago. I just wanted to check in and see if there's anything else needed on my end for this pull request. Thanks for your time!

@MarcoGorelli
Copy link
Member

thanks @IsaiasGutierrezCruz , and thanks @FBruzzesi for reviewing!

sorry this took a while - very creative solution here for pyarrow, love it! πŸ™Œ 😍

@MarcoGorelli MarcoGorelli merged commit c694148 into narwhals-dev:main Nov 12, 2024
17 checks passed
@IsaiasGutierrezCruz IsaiasGutierrezCruz deleted the dev/add_more_parameters_to_fill_null branch January 15, 2025 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enh]: include strategies "forward" and "backward" for the method "fill_null"
3 participants