Skip to content

chore: Add CompliantExpr._with_metadata#2262

Merged
MarcoGorelli merged 9 commits intonarwhals-dev:mainfrom
MarcoGorelli:with-metadata
Mar 21, 2025
Merged

chore: Add CompliantExpr._with_metadata#2262
MarcoGorelli merged 9 commits intonarwhals-dev:mainfrom
MarcoGorelli:with-metadata

Conversation

@MarcoGorelli
Copy link
Member

@MarcoGorelli MarcoGorelli commented Mar 21, 2025

This avoid the need to have kind as an argument in over, and also gives us a way to use ExprMetadata at the compliant level in other places (e.g. group_by.agg), so we can get rid of tracking function_name at the compliant level

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

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

Related issues

  • Related issue #<issue number>
  • Closes #<issue number>

Checklist

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

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

@MarcoGorelli MarcoGorelli marked this pull request as ready for review March 21, 2025 15:23
@MarcoGorelli
Copy link
Member Author

@dangotbanned I suspect (and have good reason to believe) that you can probably think of a better way to do this, in which case, feel free to add a commit to do so 🙏

@dangotbanned
Copy link
Member

@dangotbanned I suspect (and have good reason to believe) that you can probably think of a better way to do this, in which case, feel free to add a commit to do so 🙏

Thanks for the complimentary ping @MarcoGorelli 😍

I'll take a look later today

@dangotbanned dangotbanned mentioned this pull request Mar 21, 2025
20 tasks
@dangotbanned dangotbanned self-assigned this Mar 21, 2025
@dangotbanned
Copy link
Member

I'm just looking from my phone, apologies for the stray comment.

Would it make sense to add a with_metadata implementation for EagerExpr and/or LazyExpr?

You can always override it if there's something unique for a given backend (e.g. window_function).
I'd think there'd be some potential for reuse on that front

@dangotbanned
Copy link
Member

Apologies for all the questions before doing any commits 😅

Wanna make sure any changes I make are preserving your intent

- Aligns with `_with_window_function`
- Avoids stepping on *future* `pl.Expr` API (cautious)
@dangotbanned dangotbanned marked this pull request as draft March 21, 2025 18:27
@dangotbanned dangotbanned marked this pull request as ready for review March 21, 2025 18:53
@dangotbanned dangotbanned changed the title chore: Add CompliantExpr.with_metadata chore: Add CompliantExpr._with_metadata Mar 21, 2025
Copy link
Member

@dangotbanned dangotbanned left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @MarcoGorelli

All 3 of the LazyExpr constructors differ - so I only did the EagerExpr part of (#2262 (comment))

Will leave it to you for merging/making further tweaks 🙌

@MarcoGorelli
Copy link
Member Author

awesome, thanks @dangotbanned !

@MarcoGorelli MarcoGorelli merged commit e4e51b0 into narwhals-dev:main Mar 21, 2025
30 of 31 checks passed
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