Skip to content

Add more parameter defaults #1318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MarcoGorelli
Copy link
Member

Towards #1317

Due to improvements in https://github.com/yangdanny97/docs2types, further places with missing | None have emerged, I'll fix those separately

  • Closes #xxxx (Replace xxxx with the Github issue number)
  • Tests added: Please use assert_type() to assert the type of any return value

Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

As best as I can tell, the script you are using isn't creating default values for arguments that require a keyword, i.e., any argument that appears after an * in the declaration. Most of my comments are about missing defaults.

periods: int | None = ...,
start: str | DateAndDatetimeLike | None = None,
end: str | DateAndDatetimeLike | None = None,
periods: int | None = None,
*,
freq: str | timedelta | Timedelta | BaseOffset,
tz: TimeZones = ...,
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't you add defaults for the ones with ... in them above?

periods: int | None = None,
freq: str | BaseOffset | pd.Timedelta | dt.timedelta | None = None,
name: Hashable = None,
closed: IntervalClosedType = "right",
) -> IntervalIndex[Interval[pd.Timestamp]]: ...
@overload
def interval_range(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing some defaults here

@@ -348,11 +348,11 @@ def interval_range(
@overload
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing some defaults in lines 341 to 346

periods: int | None = None,
freq: str | BaseOffset | pd.Timedelta | dt.timedelta | None = None,
name: Hashable = None,
closed: IntervalClosedType = "right",
) -> IntervalIndex[Interval[pd.Timedelta]]: ...
@overload
def interval_range(
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing defaults in the parameters below

Comment on lines +46 to 47
fill_method: Literal["ffill"] | None = None,
suffixes: Suffixes = ...,
Copy link
Collaborator

Choose a reason for hiding this comment

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

suffixes has a default of ("_x", "_y")

dayfirst: bool = False,
yearfirst: bool = False,
utc: bool = False,
format: str | None = None,
exact: bool = ...,
Copy link
Collaborator

Choose a reason for hiding this comment

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

exact defaults to True

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but it documented as True and that is what happens in the code.

errors: Literal["raise", "coerce"] = ...,
downcast: _Downcast = ...,
errors: Literal["raise", "coerce"] = "raise",
downcast: _Downcast = None,
dtype_backend: DtypeBackend | _NoDefaultDoNotUse = ...,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not entirely sure whether we want to do this, but we could make the default here _NoDefaultDoNotUse

@@ -28,7 +28,7 @@ from pandas.io.parsers import TextFileReader

@overload
def read_clipboard(
sep: str | None = ...,
sep: str | None = r"\s+",
*,
dtype_backend: DtypeBackend | _NoDefaultDoNotUse = ...,
Copy link
Collaborator

Choose a reason for hiding this comment

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

the rest of the arguments to read_clipboard have the same defaults as read_csv()

@@ -176,7 +176,7 @@ def read_excel(
| OpenDocument
| pyxlsb.workbook.Workbook
),
sheet_name: int | str = ...,
sheet_name: int | str = 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you want to set the defaults for the rest of the arguments to read_excel()?

where: str | Term | Sequence[Term] | None = None,
start: int | None = None,
stop: int | None = None,
columns: list[HashableT] | None = None,
*,
iterator: Literal[True],
chunksize: int | None = ...,
Copy link
Collaborator

Choose a reason for hiding this comment

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

chunksize defaults to None here and other overloads

@MarcoGorelli
Copy link
Member Author

Yup, I've excluded keyword arguments and class functions as they'd require some extra | None fixes - would you prefer if I did that first and then ran the script all in one go?

@MarcoGorelli MarcoGorelli marked this pull request as ready for review August 13, 2025 17:52
@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Aug 13, 2025

Yup, I've excluded keyword arguments and class functions as they'd require some extra | None fixes - would you prefer if I did that first and then ran the script all in one go?

Yes, I think that's a good idea.

@MarcoGorelli MarcoGorelli marked this pull request as draft August 13, 2025 18:03
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.

2 participants