narwhals icon indicating copy to clipboard operation
narwhals copied to clipboard

feat: add more parameters to the fill_null method

Open IsaiasGutierrezCruz opened this issue 1 year ago • 2 comments

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

  • [ ] 💾 Refactor
  • [x] ✨ Feature
  • [ ] 🐛 Bug Fix
  • [ ] 🔧 Optimization
  • [ ] 📝 Documentation
  • [ ] ✅ Test
  • [ ] 🐳 Other

Related issues

  • Closes #1146

Checklist

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

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

IsaiasGutierrezCruz avatar Oct 07 '24 18:10 IsaiasGutierrezCruz

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 🤔

FBruzzesi avatar Oct 09 '24 14:10 FBruzzesi

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:

IsaiasGutierrezCruz avatar Oct 09 '24 19:10 IsaiasGutierrezCruz

I already resolved all the conversations 😃 @MarcoGorelli

IsaiasGutierrezCruz avatar Oct 20 '24 23:10 IsaiasGutierrezCruz

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

MarcoGorelli avatar Oct 21 '24 07:10 MarcoGorelli

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!

IsaiasGutierrezCruz avatar Nov 12 '24 05:11 IsaiasGutierrezCruz

thanks @IsaiasGutierrezCruz , and thanks @FBruzzesi for reviewing!

sorry this took a while - very creative solution here for pyarrow, love it! 🙌 😍

MarcoGorelli avatar Nov 12 '24 14:11 MarcoGorelli