Skip to content

Conversation

@loicdiridollou
Copy link
Member

Copy link
Contributor

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

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

Just managed to review pandas-stubs/core/indexes/interval.pyi. Will check the other two files later.

Copy link
Contributor

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

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

Please double check readers.pyi. I do not necessarily make the same suggestion in all overloads.

memory_map: bool = False,
float_precision: Literal["high", "legacy", "round_trip"] | None = None,
storage_options: StorageOptions | None = None,
dtype_backend: DtypeBackend | _NoDefaultDoNotUse = ...,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dtype_backend: DtypeBackend | _NoDefaultDoNotUse = ...,
dtype_backend: DtypeBackend | no_default = no_default,

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 those I have kept them simple ... since it would raise a lint for Only simple default values allowed for typed arguments [PYI011], let me know if you want me to go ahead and have a noqa next to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@MarcoGorelli I think you had experience with this. How would you proceed? Thanks.

Copy link
Contributor

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

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

I have reviewed all changes.

memory_map: bool = False,
float_precision: Literal["high", "legacy", "round_trip"] | None = None,
storage_options: StorageOptions | None = None,
dtype_backend: DtypeBackend | _NoDefaultDoNotUse = ...,
Copy link
Contributor

Choose a reason for hiding this comment

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

@MarcoGorelli I think you had experience with this. How would you proceed? Thanks.

@loicdiridollou
Copy link
Member Author

@cmp0xff I should have tackled all the feedback, thanks for that!
There will be a follow up PR with stubtest since I may have missed some deprecated arguments.

Copy link
Contributor

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

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

I think that's all comments from my side. Let's wait for https://github.com/pandas-dev/pandas-stubs/pull/1491/files#r2537067277.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

More defaults need to be set

2 participants