Add type hints #10

bswck · 2023-11-13T13:28:13Z

Closes #9.

bswck · 2023-11-13T13:29:22Z

I didn't look into dependencies. Please let me know how to set up the typing_extensions typing-only dependency properly in the project.

As for now, the getter's return type can't be determined. It's better to leave the return type as `Any` so that one could narrow down the getter's return type to their needs.

bswck · 2023-11-14T17:05:07Z

Ad e9788e8, please take a look at jaraco/skeleton#97.

bswck · 2023-11-14T23:30:51Z

Ad 304e455, I thought of making the classproperty class generic, but as I can see in the newest typeshed's stub for built-ins, the property class is non-generic and defines .fget's return type as Any.

bswck · 2023-11-15T15:13:59Z

This PR is part of jaraco/skeleton#98.

bswck · 2024-02-07T01:50:05Z

@jaraco waitin' for review! 🚀
Ready to adopt jaraco/skeleton#97 after.

jaraco/classes/properties.py

jaraco · 2024-02-07T14:21:33Z

I don't understand why tests are failing with "ruff: file would be formatted". The ruff formatter isn't yet enabled... or is it. Okay, yes, pytest-enabler 3.0.0 does add --ruff-format to the test config. So yes, the formatting needs to be applied.

jaraco · 2024-02-07T14:34:41Z

Doc builds are failing with:

/home/runner/work/jaraco.classes/jaraco.classes/jaraco/classes/properties.py:docstring of jaraco.classes.properties.classproperty:1: WARNING: py:class reference target not found: _CallableGetter
/home/runner/work/jaraco.classes/jaraco.classes/jaraco/classes/properties.py:docstring of jaraco.classes.properties.classproperty:1: WARNING: py:class reference target not found: _ClassMethodGetter
/home/runner/work/jaraco.classes/jaraco.classes/jaraco/classes/properties.py:docstring of jaraco.classes.properties.classproperty:1: WARNING: py:class reference target not found: _StaticMethodGetter
/home/runner/work/jaraco.classes/jaraco.classes/jaraco/classes/properties.py:docstring of jaraco.classes.properties.classproperty:1: WARNING: py:class reference target not found: _CallableSetter
/home/runner/work/jaraco.classes/jaraco.classes/jaraco/classes/properties.py:docstring of jaraco.classes.properties.classproperty:1: WARNING: py:class reference target not found: _ClassMethodSetter
/home/runner/work/jaraco.classes/jaraco.classes/jaraco/classes/properties.py:docstring of jaraco.classes.properties.classproperty.fget:1: WARNING: py:class reference target not found: _ClassMethodGetter
/home/runner/work/jaraco.classes/jaraco.classes/jaraco/classes/properties.py:docstring of jaraco.classes.properties.classproperty.fget:1: WARNING: py:class reference target not found: _StaticMethodGetter
/home/runner/work/jaraco.classes/jaraco.classes/jaraco/classes/properties.py:docstring of jaraco.classes.properties.classproperty.fset:1: WARNING: py:class reference target not found: _ClassMethodSetter
/home/runner/work/jaraco.classes/jaraco.classes/jaraco/classes/properties.py:docstring of jaraco.classes.properties.classproperty.setter:1: WARNING: py:class reference target not found: _ClassMethodSetter
/home/runner/work/jaraco.classes/jaraco.classes/jaraco/classes/properties.py:docstring of jaraco.classes.properties.classproperty.setter:1: WARNING: py:class reference target not found: _CallableSetter

These errors aren't part of the test suite, but can be invoked with tox -e docs.

These warnings, emitted as errors due to the "nitpicky" setting, are Sphinx's way of trying to indicate that it doesn't have access to the type annotations. This suggests to me that the types should be declared outside the TYPE_CHECKING block. Looks like sphinx-doc/sphinx#11225 may be relevant.

bswck · 2024-02-07T14:48:21Z

Thanks for help with the Ruff error!

I looked at the code again and noticed that I could make it better and more accurate. I'll patch my solution soon. I think I also shouldn't use NoReturn for setters... but that one I already managed to correct. Please wait a moment...

bswck · 2024-02-07T15:17:30Z

These warnings, emitted as errors due to the "nitpicky" setting, are Sphinx's way of trying to indicate that it doesn't have access to the type annotations. This suggests to me that the types should be declared outside the TYPE_CHECKING block. Looks like sphinx-doc/sphinx#11225 may be relevant.

I see. I will try with nitpick_ignore first.

bswck · 2024-02-07T16:33:50Z

These warnings, emitted as errors due to the "nitpicky" setting, are Sphinx's way of trying to indicate that it doesn't have access to the type annotations. This suggests to me that the types should be declared outside the TYPE_CHECKING block. Looks like sphinx-doc/sphinx#11225 may be relevant.

I see. I will try with nitpick_ignore first.

I found a successful work-around using nitpick_ignore:

nitpicky = True
nitpick_ignore = [
    ('py:class', 'Self'),
    ('py:class', '_T'),
    ('py:obj', 'jaraco.classes.properties._T'),
    ('py:class', '_ClassPropertyAttribute'),
    ('py:class', '_GetterCallable'),
    ('py:class', '_GetterClassMethod'),
    ('py:class', '_GetterStaticMethod'),
    ('py:class', '_SetterCallable'),
    ('py:class', '_SetterClassMethod'),
]

This is the preview I deployed from https://github.com/bswck/jaraco.classes/tree/type-hints-nitpick-ignore: https://smokeshow.helpmanual.io/6k37174k0d170s5q0h65/

I personally don't like these interfaces being unlinked, so I guess I will indeed move them out of the TYPE_CHECKING block.
@jaraco I am willing to hear your feedback on the preview, though.

bswck · 2024-02-07T16:42:07Z

Important

There is a known typing issue when trying to set values of class properties: mypy will often warn that you cannot assign to a method. The current work-around is to # type: ignore[assignment].

Should we note that somewhere? I think a relevant issue in mypy ought to be opened too, unless there is one already. Descriptors defining __set__ should be strong enough to make that error impossible.

bswck · 2024-02-07T16:52:02Z

Ah, yes, we need a requirement typing_extensions; python_version < "3.11" now, because typing_extensions gets imported at runtime.

bswck · 2024-02-07T17:08:29Z

Hmmm, that's weird. I completely removed the TYPE_CHECKING block and docs still fail with the same nitpicky-check errors.

bswck · 2024-02-07T17:11:57Z

Hmmm, that's weird. I completely removed the TYPE_CHECKING block and docs still fail with the same nitpicky-check errors.

It might be because I quoted type aliases to defer it from running at runtime... Well, let's try with the deprecated type aliases from typing then. Maybe related: sphinx-doc/sphinx#9813 (comment)

bswck · 2024-02-07T17:16:16Z

Ok, I'm honestly disappointed. Getting this to work with Sphinx will:

introduce a new dependency,
create an overhead for importing things that should be typing-only,
create a version-dependent, non-covered code branch.

@jaraco would you be OK with merging code that builds these docs in the end? It's bswck/jaraco.classes@type-hints-nitpick-ignore.

bswck · 2024-02-07T17:26:14Z

Rebased onto https://github.com/bswck/jaraco.classes/tree/type-hints-nitpick-ignore. Ready for re-review.
The new version preserves the return types of decorated staticmethods and classmethods, i.e.

class TestClassProperty(metaclass=classproperty.Meta):
    @classproperty
    @classmethod
    def foo(cls) -> str:
        return "Test"


reveal_type(TestClassProperty.foo)

reveals TestClassProperty.foo as builtins.str.

bswck · 2024-02-07T17:30:38Z

Hmmm, well... I let myself un-make the second arg to __get__ optional, as it always is passed a value according to docs.
But my change is now incompatible with PEP 252. Rolling that back.

No need for an extra type for staticmethods, it evaluates to `Callable`

jaraco · 2024-02-07T18:01:33Z

Important

There is a known typing issue when trying to set values of class properties: mypy will often warn that you cannot assign to a method. The current work-around is to # type: ignore[assignment].

Should we note that somewhere? I think a relevant issue in mypy ought to be opened too, unless there is one already. Descriptors defining __set__ should be strong enough to make that error impossible.

🤷 Ideally, there'd be an upstream issue, but I'll leave it to you to decide if that's worth the investment.

jaraco

LGTM. Thanks for working through all the issues.

jaraco/classes/properties.py

bswck · 2024-02-07T18:38:19Z

Important
There is a known typing issue when trying to set values of class properties: mypy will often warn that you cannot assign to a method. The current work-around is to # type: ignore[assignment].
Should we note that somewhere? I think a relevant issue in mypy ought to be opened too, unless there is one already. Descriptors defining __set__ should be strong enough to make that error impossible.

🤷 Ideally, there'd be an upstream issue, but I'll leave it to you to decide if that's worth the investment.

The error is easy to silence with # type: ignore[assignment] and I will maybe open an issue in the future. As for now, what we have seems to work out just fine.

bswck · 2024-02-07T18:47:36Z

One more pedantic improvement 😅
I think we're ready to merge?

bswck added 4 commits November 13, 2023 14:14

Add type hints to jaraco.classes.meta

2788752

Add type hints to jaraco.classes.ancestry

67d4490

Add type hints to jaraco.classes.properties

b45bde0

Add py.typed marker

acbe069

bswck changed the title ~~Add type hints to jaraco.classes.meta~~ Add type hints Nov 13, 2023

bswck marked this pull request as ready for review November 13, 2023 13:28

bswck added 4 commits November 13, 2023 14:32

Use single-quote strings for consistency

f9f6f74

Move Callable import into type-checking block

5b84b54

Make classproperty.__get__() return Any

304e455

As for now, the getter's return type can't be determined. It's better to leave the return type as `Any` so that one could narrow down the getter's return type to their needs.

Tweak coverage configuration for type checking

e9788e8

Merge branch 'main' into type-hints

5c45b96

jaraco reviewed Feb 7, 2024

View reviewed changes

jaraco/classes/properties.py Outdated Show resolved Hide resolved

jaraco approved these changes Feb 7, 2024

View reviewed changes

👹 Feed the hobgoblins (delint).

95d78a5

In TODO, link an issue, not a person

b68233d

bswck added 2 commits February 7, 2024 17:18

Use None return type for setters, rename type aliases

e6206f5

Nitpick ignore TYPE_CHECKING block types

135f192

bswck force-pushed the type-hints branch from e88b710 to 7fa4cb3 Compare February 7, 2024 17:00

bswck force-pushed the type-hints branch from 67148d9 to 7fa4cb3 Compare February 7, 2024 17:05

Use Protocol[_T] as _ClassPropertyAttribute base

b1e7325

bswck force-pushed the type-hints branch from 4144206 to e6206f5 Compare February 7, 2024 17:25

PEP 252 compatibility

d2d54e8

bswck requested a review from jaraco February 7, 2024 17:34

bswck force-pushed the type-hints branch from f36d1df to d2d54e8 Compare February 7, 2024 17:45

Remove _GetterStaticMethod type

8408287

No need for an extra type for staticmethods, it evaluates to `Callable`

jaraco approved these changes Feb 7, 2024

View reviewed changes

jaraco/classes/properties.py Show resolved Hide resolved

Prefer single quotes

032ecc1

jaraco merged commit bf6dac1 into jaraco:main Feb 7, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type hints #10

Add type hints #10

bswck commented Nov 13, 2023 •

edited

bswck commented Nov 13, 2023 •

edited

bswck commented Nov 14, 2023

bswck commented Nov 14, 2023

bswck commented Nov 15, 2023 •

edited

bswck commented Feb 7, 2024

jaraco commented Feb 7, 2024

jaraco commented Feb 7, 2024

bswck commented Feb 7, 2024 •

edited

bswck commented Feb 7, 2024

bswck commented Feb 7, 2024 •

edited

bswck commented Feb 7, 2024

bswck commented Feb 7, 2024 •

edited

bswck commented Feb 7, 2024

bswck commented Feb 7, 2024 •

edited

bswck commented Feb 7, 2024 •

edited

bswck commented Feb 7, 2024 •

edited

bswck commented Feb 7, 2024 •

edited

jaraco commented Feb 7, 2024

jaraco left a comment

bswck commented Feb 7, 2024

bswck commented Feb 7, 2024

Add type hints #10

Add type hints #10

Conversation

bswck commented Nov 13, 2023 • edited

bswck commented Nov 13, 2023 • edited

bswck commented Nov 14, 2023

bswck commented Nov 14, 2023

bswck commented Nov 15, 2023 • edited

bswck commented Feb 7, 2024

jaraco commented Feb 7, 2024

jaraco commented Feb 7, 2024

bswck commented Feb 7, 2024 • edited

bswck commented Feb 7, 2024

bswck commented Feb 7, 2024 • edited

bswck commented Feb 7, 2024

bswck commented Feb 7, 2024 • edited

bswck commented Feb 7, 2024

bswck commented Feb 7, 2024 • edited

bswck commented Feb 7, 2024 • edited

bswck commented Feb 7, 2024 • edited

bswck commented Feb 7, 2024 • edited

jaraco commented Feb 7, 2024

jaraco left a comment

Choose a reason for hiding this comment

bswck commented Feb 7, 2024

bswck commented Feb 7, 2024

bswck commented Nov 13, 2023 •

edited

bswck commented Nov 13, 2023 •

edited

bswck commented Nov 15, 2023 •

edited

bswck commented Feb 7, 2024 •

edited

bswck commented Feb 7, 2024 •

edited

bswck commented Feb 7, 2024 •

edited

bswck commented Feb 7, 2024 •

edited

bswck commented Feb 7, 2024 •

edited

bswck commented Feb 7, 2024 •

edited

bswck commented Feb 7, 2024 •

edited