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 when-then-otherwise expression #588

Merged
merged 92 commits into from
Aug 24, 2024

Conversation

aivanoved
Copy link
Contributor

@aivanoved aivanoved commented Jul 23, 2024

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.

This PR so far adds a simple when-then-otherwise expression with no chaining

The PR does not aim to be complete as of now, the current aim is to get an initial review as to if this approach is consistent with the project's quality and style of code

TODO:

Note: Temporary converted back to draft due to a git rebase issue fixed

Note: due to an unfortunate rebase gone wrong, the commit history is a mess, we could open a new pr from a new branch or when merging do a squash merge. Further advice on this would be much appreciated.

@aivanoved aivanoved marked this pull request as ready for review July 23, 2024 10:15
@aivanoved aivanoved marked this pull request as draft July 23, 2024 12:44
@aivanoved aivanoved changed the title Add where expression Add when-then-otherwise expression Jul 23, 2024
@aivanoved
Copy link
Contributor Author

aivanoved commented Aug 16, 2024

fixed by xfail current ci fails with the new zip_with, due to a known issue in modin, here

fixed ci fails because of this if conditioning, which is essentially a pandas version check, so the else condition does not get executed for pandas>=1.*, for me this is a tricky one to fix.failure due to coverage test

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.

thanks @aivanoved ! I just pushed some slight changes to avoid using nullable dtypes when the user didn't ask for them

@MarcoGorelli MarcoGorelli merged commit 83dd6e1 into narwhals-dev:main Aug 24, 2024
21 checks passed
@EdAbati
Copy link
Contributor

EdAbati commented Aug 25, 2024

Thank you @aivanoved for working this! This is a great addition

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.

5 participants