Skip to content
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

type is_dtype_equal #1112

Merged
merged 1 commit into from
Mar 4, 2025
Merged

Conversation

MarcoGorelli
Copy link
Member

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

There's already a test for this one using assert_type;

def test_is_dtype_equal() -> None:
check(assert_type(api.is_dtype_equal("i4", np.int8), bool), bool)

the function just wasn't typed

@MarcoGorelli MarcoGorelli marked this pull request as ready for review February 8, 2025 12:45
@MarcoGorelli MarcoGorelli changed the title type is-dtype-equal type is_dtype_equal Feb 8, 2025
@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Feb 10, 2025

In the stubs, we don't include type definitions for methods that are not documented. In this case, is_dtype_equal() is not documented, so I'd prefer to just delete the line from the stubs.

On the other hand, if you think it should be documented, then you can raise an issue in the pandas repo, and we make a decision about this PR after the doc issue is resolved.

@MarcoGorelli
Copy link
Member Author

I'd prefer to just delete the line from the stubs.

sure thing, always happy to delete code

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.

We have references to is_dtype_equal() in pandas-stubs\api\types\__init__.pyi and pandas-stubs\core\dtypes\api.pyi that should be removed as well.

@@ -28,7 +28,7 @@
)

from pandas.core.dtypes.base import ExtensionDtype
from pandas.core.dtypes.common import (
from pandas.core.dtypes.common import ( # type: ignore[attr-defined]
Copy link
Collaborator

Choose a reason for hiding this comment

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

UGH. Now that I see that this is used in a test (this one is copied from the pandas source), I think we need to open an issue in pandas about documenting this. I'll do that, and keep this PR on hold.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Dr-Irv Dr-Irv added the pandas_docs For issues where there is a conflict in behavior with pandas docs and stubs that needs resolution label Feb 10, 2025
@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Feb 26, 2025

@MarcoGorelli ok to change this from draft - it will get documented

@MarcoGorelli MarcoGorelli marked this pull request as ready for review March 3, 2025 14:42
@MarcoGorelli MarcoGorelli marked this pull request as draft March 3, 2025 14:42
@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Mar 3, 2025

@MarcoGorelli pinging you again about whether you want to take this out of draft status

@MarcoGorelli MarcoGorelli marked this pull request as ready for review March 4, 2025 10:28
@MarcoGorelli
Copy link
Member Author

sure, thanks - so, undraft for now and then we type it once it's documented?

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Mar 4, 2025

sure, thanks - so, undraft for now and then we type it once it's documented?

Sorry - my mistake. Since we decided to document it, let's type it for now, which was the original PR here.

@MarcoGorelli MarcoGorelli force-pushed the type-is-dtype-equal branch from 47c837d to cda4357 Compare March 4, 2025 18:52
@MarcoGorelli
Copy link
Member Author

sure, done, thanks!

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.

thanks @MarcoGorelli

@Dr-Irv Dr-Irv merged commit 5f22e39 into pandas-dev:main Mar 4, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pandas_docs For issues where there is a conflict in behavior with pandas docs and stubs that needs resolution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants