Skip to content

Conversation

@dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Sep 4, 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

@dangotbanned dangotbanned added documentation Improvements or additions to documentation internal typing labels Sep 4, 2025
dangotbanned added a commit that referenced this pull request Sep 5, 2025
- Follow-up to #3016
- Doesn't quite fit into #3086
FBruzzesi pushed a commit that referenced this pull request Sep 6, 2025
- Follow-up to #3016
- Doesn't quite fit into #3086
@dangotbanned dangotbanned marked this pull request as ready for review September 9, 2025 17:31
@dangotbanned
Copy link
Member Author

@FBruzzesi just pinging you on the off-chance you can review before I'm stranded πŸ˜‰

@FBruzzesi
Copy link
Member

@FBruzzesi just pinging you on the off-chance you can review before I'm stranded πŸ˜‰

Hey @dangotbanned I will take a look before the end of the weekend/I leave for vacation :D

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 @dangotbanned - I didn't check line by line, I assumed that most of the changes were simply due to the different import source and somplifications. I double checked the new module and left a couple of comments. Let me know what your thoughts are

Comment on lines 5 to 37
### (1) `Native(*Frame|Series)`
Minimal [`Protocol`]s for matching *almost any* supported native type of that group:
class NativeThing(Protocol):
def something_common(self, *args: Any, **kwargs: Any) -> Any: ...
Note:
This group is primarily a building block for more useful types.
### (2) `Into(*Frame|Series)`
*Publicly* exported [`TypeAlias`]s of **(1)**:
IntoThing: TypeAlias = NativeThing
**But**, occasionally, there'll be an edge-case which we can spell like:
IntoThing: TypeAlias = Union[<type that does not fit the protocol>, NativeThing]
Tip:
Reach for these when there **isn't a need to preserve** the original native type.
### (3) `Into(*Frame|Series)T`
*Publicly* exported [`TypeVar`]s, bound to **(2)**:
IntoThingT = TypeVar("IntoThingT", bound=IntoThing)
Important:
In most situations, you'll want to use these as they **do preserve** the original native type.
Putting it all together, we can now add a *narwhals-level* wrapper:
class Thing(Generic[IntoThingT]):
def to_native(self) -> IntoThingT: ...
Copy link
Member

Choose a reason for hiding this comment

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

Mostly nitpicking on the usage of Thing. I am trying to put myself into the shoes of a newcomer and I wonder if something along the following lines would be more helpful:

Suggested change
### (1) `Native(*Frame|Series)`
Minimal [`Protocol`]s for matching *almost any* supported native type of that group:
class NativeThing(Protocol):
def something_common(self, *args: Any, **kwargs: Any) -> Any: ...
Note:
This group is primarily a building block for more useful types.
### (2) `Into(*Frame|Series)`
*Publicly* exported [`TypeAlias`]s of **(1)**:
IntoThing: TypeAlias = NativeThing
**But**, occasionally, there'll be an edge-case which we can spell like:
IntoThing: TypeAlias = Union[<type that does not fit the protocol>, NativeThing]
Tip:
Reach for these when there **isn't a need to preserve** the original native type.
### (3) `Into(*Frame|Series)T`
*Publicly* exported [`TypeVar`]s, bound to **(2)**:
IntoThingT = TypeVar("IntoThingT", bound=IntoThing)
Important:
In most situations, you'll want to use these as they **do preserve** the original native type.
Putting it all together, we can now add a *narwhals-level* wrapper:
class Thing(Generic[IntoThingT]):
def to_native(self) -> IntoThingT: ...
### (1) `Native(*Frame|Series)`
Minimal [`Protocol`]s for matching *almost any* supported native type of that group:
class NativeObject(Protocol):
def some_common_method(self, *args: Any, **kwargs: Any) -> Any: ...
Note:
This group is primarily a building block for more useful types.
### (2) `Into(*Frame|Series)`
*Publicly* exported [`TypeAlias`]s of **(1)**:
IntoNarwhalsObject: TypeAlias = NativeObject
**But**, occasionally, there'll be an edge-case which we can spell like:
IntoNarwhalsObject: TypeAlias = Union[<type that does not fit the protocol>, NativeObject]
Tip:
Reach for these when there **isn't a need to preserve** the original native type.
### (3) `Into(*Frame|Series)T`
*Publicly* exported [`TypeVar`]s, bound to **(2)**:
IntoNarwhalsObjectT = TypeVar("IntoNarwhalsObjectT", bound=IntoNarwhalsObject)
Important:
In most situations, you'll want to use these as they **do preserve** the original native type.
Putting it all together, we can now add a *narwhals-level* wrapper:
class NarwhalsObject(Generic[IntoNarwhalsObjectT]):
def to_native(self) -> IntoNarwhalsObjectT: ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review @FBruzzesi!

Mostly nitpicking on the usage of Thing

I am tempted to bikeshed on the name πŸ˜„ ...

But regardless of what exactly it is - would some kind of intro like this be helpful?

"""[Protocols] defining conversion methods between representations.
These come in 3 flavors and are [generic] to promote reuse.
The following examples use the placeholder types `Narwhal` and `Other`:
- `Narwhal`: some class written in `narwhals`.
- `Other`: any other class, could be native, compliant, or a builtin.
## `To<Other>`
When we want to convert or unwrap a `Narwhal` into an `Other`,
we provide an **instance** method:

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think it might help, but really I don't want to make a fuss out of it - I am not a fan of using a generic thing, and in this situation (i.e. a python context?!) *Class or *Object would be more appropriate πŸ˜‚

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 am tempted to bikeshed on the name πŸ˜„ ...
But regardless of what exactly it is

Sorry @FBruzzesi, I'm reading this back now and it isn't coming across the way that I intended.

My suggestion (#3086 (comment)) was supposed to be another way to help with

I am trying to put myself into the shoes of a newcomer

But not saying that should be instead of choosing a better name than *Thing πŸ™‚

Copy link
Member Author

Choose a reason for hiding this comment

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

So the naming scheme in (#3086 (comment)) is slightly different from what I was trying to demonstrate

and in this situation (i.e. a python context?!) *Class or *Object would be more appropriate πŸ˜‚

Here's what each of those look like:

Actual Current Proposed Alt 1 Alt 2
NativeSeries NativeThing NativeObject NativeObject NativeClass
IntoSeries IntoThing IntoNarwhalsObject IntoObject IntoClass
IntoSeriesT IntoThingT IntoNarwhalsObjectT IntoObjectT IntoClassT
Series Thing NarwhalsObject Object Class

Note

FWIW, I don't like any of them now - including Thing 😭

Copy link
Member

Choose a reason for hiding this comment

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

At the end of the day it's a developer documentation, not even visible to a user so I won't be too picky nor a blocker.

If everything else is okay - we could do one more round of brainstorming a better name than Thing to get it over the line ☺️

For iteration over it, I am quite open to anything which is not called Thing πŸ˜‚

Copy link
Member Author

Choose a reason for hiding this comment

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

For iteration over it, I am quite open to anything which is not called Thing πŸ˜‚

Alright I'll try to churn out a list of non-Things to choose from shortly πŸ˜‰

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay @FBruzzesi here we go:

  • Array
  • Data
  • Column
  • Matrix
  • Dog
  • Scalar
  • LinkedList
  • Box
  • Bear

Copy link
Member

Choose a reason for hiding this comment

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

@dangotbanned I am sold! Let's ship it with Bear's and Dog's πŸ˜‚

/sarcam


Really feel free to merge after resolving the conflict

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @FBruzzesi 😍

If you decide on one you prefer (including Bear and Dog πŸ˜‰), feel free to update the doc in a follow up πŸ™Œ

dangotbanned added a commit that referenced this pull request Oct 1, 2025
@dangotbanned
Copy link
Member Author

@FBruzzesi how do you feel about the rest of this besides (#3086 (comment))?

I really want to move forward on it to unblock #3125 and I also noticed (main...chore/refactor-pandas-like-pyarrow-branching) would be touching some of the changed bits as well

If everything else is okay - we could do one more round of brainstorming a better name than Thing to get it over the line ☺️

@FBruzzesi
Copy link
Member

@FBruzzesi how do you feel about the rest of this besides (#3086 (comment))?

I really want to move forward on it to unblock #3125 and I also noticed (main...chore/refactor-pandas-like-pyarrow-branching) would be touching some of the changed bits as well

If everything else is okay - we could do one more round of brainstorming a better name than Thing to get it over the line ☺️

Sorry @dangotbanned I went on vacation and somehow thought this PR was done. I will take another look in the weekend

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 @dangotbanned - I left a nitpick suggestion, everything else looks good to me. Sorry for the headache over naming things

Co-authored-by: Francesco Bruzzesi <[email protected]>
@MarcoGorelli
Copy link
Member

thanks for your PR!

if you're both happy with it feel free to take it forwards


it wasn't clear to me on reading why we're now returning Into* instead of Native* in the tests but perhaps it'll become clear if i expand all the convos

@dangotbanned
Copy link
Member Author

dangotbanned commented Oct 17, 2025

thanks for your PR!

if you're both happy with it feel free to take it forwards

Thanks @MarcoGorelli 😍

it wasn't clear to me on reading why we're now returning Into* instead of Native* in the tests but perhaps it'll become clear if i expand all the convos

Hmm, I seem to have done a bad job on the docs then πŸ˜…

Here I was trying to explain ...

### (2) `Into<Thing>`
*Publicly* exported [`TypeAlias`]s of **(1)**:
IntoThing: TypeAlias = NativeThing
**But**, occasionally, there'll be an edge-case which we can spell like:
IntoThing: TypeAlias = Union[<type that does not fit the protocol>, NativeThing]
Tip:
Reach for these when there **isn't a need to preserve** the original native type.

... which means we can write like this

IntoDataFrame: TypeAlias = Union["NativeDataFrame", "DataFrameLike"]

IntoDataFrame: TypeAlias = NativeDataFrame

IntoLazyFrame: TypeAlias = "NativeLazyFrame"

IntoLazyFrame: TypeAlias = Union[NativeLazyFrame, NativeIbis]

This wasn't the original purpose of the aliases, but I've kinda reclaimed them to work as a lil barrier between the protocols

@MarcoGorelli
Copy link
Member

cool, thanks!

Hmm, I seem to have done a bad job on the docs then πŸ˜…

oh no, you've done an excellent job, i just hadn't spent much time on it, happy to trust you two on this one

@dangotbanned dangotbanned merged commit 556c6a6 into main Oct 20, 2025
31 of 34 checks passed
@dangotbanned dangotbanned deleted the refac-native-module branch October 20, 2025 09:10
dangotbanned added a commit that referenced this pull request Oct 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation internal typing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants