-
-
Notifications
You must be signed in to change notification settings - Fork 17.4k
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
STYLE #49656: fixed redefined-outer-name linting issue to format.py #49937
Conversation
…class __call__ function
…d outer name conflict with is_dates_only function
…ange in datetimes.py
…er name conflict with justify functions defined for TextAdjustment and EastAsianTextAdjustment classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MarieKMiko for working on this! Could you also do
I've left a couple of comments - could you also remove file names you've fixed from
pandas/.pre-commit-config.yaml
Lines 82 to 84 in 094b2c0
|^pandas/core/tools/datetimes\.py | |
|^pandas/io/formats/format\.py | |
|^pandas/core/generic\.py |
please?
pandas/io/formats/format.py
Outdated
@@ -1794,12 +1794,12 @@ def _format_datetime64_dateonly( | |||
|
|||
|
|||
def get_format_datetime64( | |||
is_dates_only: bool, nat_rep: str = "NaT", date_format: str | None = None | |||
is_dates_only_result: bool, nat_rep: str = "NaT", date_format: str | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally, let's not rename function arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed this is not ideal- not sure how to account for the issue that is_dates_only is both a function name and an argument in a different function, which seems to be what's causing the issue. Would it be better to rename the function rather than then argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see what you mean, thanks. gosh, that's dirty 😄 taking a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is_dates_only
is only used within this file - how about we just rename it to _is_dates_only
, and then this argument can stay as it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that could work - let me double check because there is another function somewhere called _is_dates_only but it may be in a different file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is_dates_only is only used within this file
sorry I was wrong, it's used inside that other function in fact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In
pandas/pandas/io/formats/style_render.py
Line 1806 in 694c0cd
def non_reducing_slice(slice_: Subset): |
the argument slice
is called slice_
to avoid shadowing the builtin slice
- shall we do the same here and rename to is_dates_only_
?
If you then remove format.py
from
pandas/.pre-commit-config.yaml
Line 83 in 61e0db2
|^pandas/io/formats/format\.py |
file, the rest looks good 👍
EDIT: in fact, looks like |^pandas/core/tools/datetimes\.py
can also already be removed from the pre-commit-config file
pandas/io/formats/format.py
Outdated
@@ -1794,12 +1794,12 @@ def _format_datetime64_dateonly( | |||
|
|||
|
|||
def get_format_datetime64( | |||
is_dates_only: bool, nat_rep: str = "NaT", date_format: str | None = None | |||
is_dates_only_result: bool, nat_rep: str = "NaT", date_format: str | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In
pandas/pandas/io/formats/style_render.py
Line 1806 in 694c0cd
def non_reducing_slice(slice_: Subset): |
the argument slice
is called slice_
to avoid shadowing the builtin slice
- shall we do the same here and rename to is_dates_only_
?
If you then remove format.py
from
pandas/.pre-commit-config.yaml
Line 83 in 61e0db2
|^pandas/io/formats/format\.py |
file, the rest looks good 👍
EDIT: in fact, looks like |^pandas/core/tools/datetimes\.py
can also already be removed from the pre-commit-config file
…_datetime64 to is_dates_only
pandas/io/formats/format.py
Outdated
@@ -1744,7 +1742,7 @@ def format_percentiles( | |||
return [i + "%" for i in out] | |||
|
|||
|
|||
def is_dates_only(values: np.ndarray | DatetimeArray | Index | DatetimeIndex) -> bool: | |||
def is_dates_only_(values: np.ndarray | DatetimeArray | Index | DatetimeIndex) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah sorry, I meant to rename the argument get_format_datetime64
(else this function will need renaming in the other file where it's used, i.e.
pandas/pandas/core/indexes/datetimes.py
Line 359 in 14dc069
from pandas.io.formats.format import is_dates_only |
so, this function would stay as is_dates_only
, and
pandas/pandas/io/formats/format.py
Lines 1796 to 1798 in 1c51e60
def get_format_datetime64( | |
is_dates_only: bool, nat_rep: str = "NaT", date_format: str | None = None | |
) -> Callable: |
would change to
def get_format_datetime64(
is_dates_only_: bool, nat_rep: str = "NaT", date_format: str | None = None
) -> Callable:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh- got it- let me just swap that around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This is probably fine, if you address #49937 (review) as well then this is probably good to go
removed format and datetimes files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MarieKMiko |
Fixed redefined-outer-name issue in:
redefined-outer-name t issues raised for:
justify function (import from pandas.io.formats.printing)
decimal (changed to from decimal import Decimal)
is_dates_only parameter conflict with function name
updated get_format_datetime64 function with renamed parameter
passed related tests in pytest pandas
running: pylint --disable=all --enable=redefined-outer-name pandas/io/
-- > Results in 10.00/10 rating