Skip to content

chore: Tidy up PolarsExpr#2313

Merged
MarcoGorelli merged 5 commits intomainfrom
polars-expr-native
Mar 29, 2025
Merged

chore: Tidy up PolarsExpr#2313
MarcoGorelli merged 5 commits intomainfrom
polars-expr-native

Conversation

@dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Mar 29, 2025

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

Missed during the linked PRs/issues, but noticed in (#2312)

Comment on lines +93 to +97
if self._backend_version >= (1, 18):
native = self.native.is_nan()
else: # pragma: no cover
native = pl.when(self.native.is_not_null()).then(self.native.is_nan())
return self._with_native(native)
Copy link
Member Author

Choose a reason for hiding this comment

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

I flipped the order to avoid a warning from mypy.

  • The second branch is pl.Then instead of pl.Expr
  • That's fine this way, because it is a subclass of pl.Expr

Copy link
Member Author

Choose a reason for hiding this comment

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

More generally though, using >= is more common outside of narwhals.

Although I'm not sure how to rewrite these rules to work that way - so no biggie 🙂

narwhals/pyproject.toml

Lines 255 to 266 in 5550ad8

exclude_also = [
"if sys.version_info() <",
"if .*implementation is Implementation.CUDF",
"if .*implementation is Implementation.PYSPARK",
"if .*implementation.is_cudf",
"if .*implementation.is_pyspark",
'request.applymarker\(pytest.mark.xfail',
'backend_version <',
'if "cudf" in str\(constructor',
'if "pyspark" in str\(constructor',
'pytest.skip\('
]

@dangotbanned dangotbanned marked this pull request as ready for review March 29, 2025 15:31
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.

very nice, thanks @dangotbanned !

@MarcoGorelli MarcoGorelli merged commit 6d5ad4d into main Mar 29, 2025
30 checks passed
@MarcoGorelli MarcoGorelli deleted the polars-expr-native branch March 29, 2025 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants