CLN: remove kwargs in Index.format #35122
Conversation
topper-123
force-pushed
the
cln_index.format
branch
2 times, most recently
from
51cbd77 to
31c4eb7
Compare
pandas/core/indexes/multi.py
Outdated
| na_rep=None, | ||
| formatter=None, | ||
| ): | ||
| ) -> list: |
There was a problem hiding this comment.
Can you use List from typing and add a subscript?
There was a problem hiding this comment.
This has return type List[Union[str, Tuple[str]]]. It returns List[Tuple[str]] if adjoin=False, else List[str].
I'm not a fan of union return types, it can trip mypy up in random places. I would prefer to use Literal types and typing.cast, but that is only available in python 3.8. Hope that's ok?
There was a problem hiding this comment.
Sparsify is actually of type Union[bool, None, Literal[lib.no_default]], which can't be represented until python 3.8. I'll just leave it as is.
There was a problem hiding this comment.
I think should use the union or maybe even overload based off of your comment (@simonjayhawkins has used overload successfully in a few places already); makes for much better type checking than a generic list
There was a problem hiding this comment.
I agree, we don't use a bare list as typing anywhere else, I would change this (and use a Union)
pandas/core/indexes/multi.py
Outdated
| @@ -1231,13 +1231,17 @@ def _format_native_types(self, na_rep="nan", **kwargs): | |||
|
|
|||
| def format( | |||
| self, | |||
| name=None, | |||
topper-123
force-pushed
the
cln_index.format
branch
from
31c4eb7 to
ef4732a
Compare
|
Hello @topper-123! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-07-14 20:50:39 UTC |
| if sparsify not in [True, 1]: | ||
| # GH3547 use value of sparsify as sentinel if it's "Falsey" | ||
| assert isinstance(sparsify, bool) or sparsify is lib.no_default | ||
| if sparsify in [False, lib.no_default]: |
There was a problem hiding this comment.
Clarifying possible values here.
topper-123
force-pushed
the
cln_index.format
branch
from
af3016f to
ba17b28
Compare
|
Updated. @WillAyd? |
|
Ping. |
topper-123
force-pushed
the
cln_index.format
branch
from
ba17b28 to
43f0727
Compare
|
I missed that, I've updated again. |
|
thanks @topper-123 |
Removes kwargs in
Index.formatand subclasses. Also changesIndex._format_with_headerto have parameterna_repnot have a default value (in order to not have default value set in two locations).Related to #35118.