Skip to content

Conversation

FBruzzesi
Copy link
Member

@FBruzzesi FBruzzesi commented Jul 26, 2025

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

  • πŸ’Ύ Refactor
  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ”§ Optimization
  • πŸ“ Documentation
  • βœ… Test
  • 🐳 Other

Checklist

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

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

Related issue: #2722

I am slightly concerned that this could turn out to be a month long PR. I will open it as draft for now. There are a couple of pain points I already know:

  • I was very careful with type hints, yet it somehow doesn't pass type checker Fixed
  • A custom check (importing from dtypes) will fail. That's the default used in the dtype argument. In principle we could just ignore the argument completely and always opt for Int64.
  • Polars can do eager: bool. In our case it's a bit more complex than that. Instead of adding yet another argument to the function, I allowed for eager to be False (default), None or the backend/implementation that should back the series.

@FBruzzesi FBruzzesi added enhancement New feature or request pyarrow Issue is related to pyarrow backend pandas-like Issue is related to pandas-like backends polars Issue is related to polars backend labels Jul 26, 2025
@FBruzzesi FBruzzesi marked this pull request as ready for review July 27, 2025 09:17
@dangotbanned
Copy link
Member

dangotbanned commented Jul 27, 2025

Hey @FBruzzesi, as I've mentioned 8347839 times, I'm very keen to see this in narwhals! πŸ˜„

Would you be okay if we let this one marinate for a bit and maybe target the release after the next? πŸ™

@FBruzzesi
Copy link
Member Author

Hey @FBruzzesi,as I've mentioned 8347839 times, I'm very keen to see this in narwhals! πŸ˜„

πŸŽ‰

Would you be okay if we let this one marinate for a bit and maybe target the release after the next? πŸ™

Yeah I was not expecting this to be finalized by tomorrow. It has some sharp edges that need some love and work

@FBruzzesi
Copy link
Member Author

@MarcoGorelli any "trick" to avoid the custom check in precommit?

  • For compliant namespaces the import of IntegerType is in the if TYPE_CHECKING block
  • For function.py, Int64 is used as default value (and then overwritten by the v*.dtypes.Int64 dtype). I guess one way of doing it is to let it be None and set it inside the function given the version. Yet It's a bit less explicit to a user.

I thought about emulating that utils/import_check.py does, but I would need to figure out what's happening there from scratch πŸ˜‚

@dangotbanned
Copy link
Member

any "trick" to avoid the custom check in precommit?

You could add this to nw.typing, and then use that everywhere instead?

IntegerDType: TypeAlias = "dtypes.IntegerType | type[dtypes.IntegerType]"

@dangotbanned dangotbanned removed pandas-like Issue is related to pandas-like backends polars Issue is related to polars backend pyarrow Issue is related to pyarrow backend labels Aug 5, 2025
@dangotbanned dangotbanned marked this pull request as draft August 20, 2025 17:18
@dangotbanned dangotbanned marked this pull request as ready for review August 20, 2025 17:31
@dangotbanned
Copy link
Member

this is probably fine, i'd just like to take a look before merging please, thanks πŸ™

@MarcoGorelli pinging again as (#2895 (comment)) was a month ago 🫣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
eager-only enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support int_range (Eager-only)
3 participants