Add type hints #22

bswck · 2023-07-06T04:02:11Z

Added type hints and the necessary py.typed marker (which would probably affect other jaraco/... modules(?), so please let me know whether I should revert adding that)
Honestly those type hints slightly interfered with readability, so please let me know if I should rather put those into a stub file.
Changed the docs configuration to ignore P and R (previously CallableT was ignored). Could you please check if docs build as they should? I didn't figure out how to build it myself
Added conditional typing_extensions dependency for Python versions <=3.10 (Concatenate and ParamSpec are used)

And a bonus

Added identity() documentation
Deprecated method_caller, now redirects to operator.methodcaller

jaraco

Thanks for the contrib.

Other changes I observed in my review:

Reformatted the docstrings to prefer putting the content on the first line instead of aligning left.
Restyled a docstring to replace a sentence with a section heading.
Made some None return types explicit.
Whitespace tweaks.
Updated docstrings to make the first line a proper sentence ending in a period.

For the future, please consider separating stylistic changes grouped into separate commits so they can be cherry-picked or excluded easily.

For example, I prefer the docstrings to start at the same level of indentation, as allowed by PEP 257. Changing the docstrings here implies that they should be changed across the 100 other projects I maintain and I'm not ready to accept that responsibility (and probably won't until a tool exists to autoformat that I can adopt programmatically, such as in jaraco/skeleton).

I'll roll back that change, but otherwise, this looks great, with just a few other minor things to consider besides the failing test, the most pressing concern.

jaraco/functools.py

jaraco · 2023-08-06T01:05:03Z

jaraco/functools.py

-    def reset(self):
-        self.last_called = 0
+    def reset(self) -> None:
+        self.last_called = 0.0


Let's avoid changing the implementation when adding type annotations. Let's re-use the return type for time.time() and the input type for time.sleep(). Also below.

jaraco/functools.py

jaraco · 2023-08-07T13:51:07Z

which would probably affect other jaraco/... modules(?)

I don't know the answer to this either. Here's my thinking:

I'm not opposed to having all jaraco.* modules tagged with py.typed, as long as that doesn't introduce a massive amount of toil across those projects.
If it is too onerous to add the file at the jaraco package, the proper fix would be to move jaraco/functools.py to jaraco/functools/__init__.py.
However, now that I think about it, it's a bad idea and unsupported for namespace packages to present the same file. It causes unpredictable results, especially when various install/uninstall scenarios are considered. Moreover, it would create scenarios where some jaraco.* packages are considered "typed" only when jaraco.functools is installed. That can't be good.

And in fact, the PEP addresses this concern specifically:

For namespace packages (see PEP 420), the py.typed file should be in the submodules of the namespace, to avoid conflicts and for clarity.

...

The single-file module should be refactored into a package and indicate that the package supports typing...

So yes, the proper solution is to move functools into its own package.

bswck

Thanks for this excellent review. Sorry for cumulating many types of changes in one commit. I will pay attention to it in the future PRs, including those to bring type annotations to other jaraco.* projects :)

I've applied the suggested changes in docstrings and comments.
The remaining requests relate to type annotations being added in the runtime module, so do you mind if I go ahead and create a stub file and migrate all annotations there instead of resolving these requests for now?

I'm thinking of making jaraco.functools a package by the way when moving the annotations to the stub file.

jaraco/functools.py

jaraco · 2023-08-12T00:10:10Z

do you mind if I go ahead and create a stub file and migrate all annotations there instead of resolving these requests for now?

Fine by me. Thanks!

I'm thinking of making jaraco.functools a package by the way when moving the annotations to the stub file.

Also fine. Let me know if it would help for me to do this in main first.

Please let me know if I should revert this here and suggest implementation changes in a separate PR if you don't mind.

I don't know why I can't reply to the thread, but yes, please propose behavioral changes in a separate change (although I think float() is probably unnecessary, I'd be interested in your perspective and the value proposition).

bswck · 2023-08-14T05:54:11Z

Also fine. Let me know if it would help for me to do this in main first.

Thanks :) I think it's fine if it's done in this branch (since we migrate to the package format only due to the added type hints) unless you find any other reason to apply the change in main beforehand. My changes are ready, submitted below.
Update: #22 (comment)

I don't know why I can't reply to the thread, but yes, please propose behavioral changes in a separate change (although I think float() is probably unnecessary, I'd be interested in your perspective and the value proposition).

I don't know either. Sorry, I must have messed something up.

Sure, I will propose the changes separately. (float() cast was added due to my changing the max_rate parameter type from float to SupportsFloat. Honestly though I have something more to add to the change, i.e. accepting max_rate as None for more convenience, but that's the topic for the proper PR that I am going to submit later).

bswck · 2023-08-14T06:17:12Z

Tested against mypy --strict and stubtest with successful results. Pylance reports some problems in the implementation which can be addressed now or in a separate PR (see #22 (comment)).

[{
	"resource": "/home/bswck/dev/jaraco.functools/jaraco/functools/__init__.py",
	"owner": "_generated_diagnostic_collection_name_#2",
	"code": {
		"value": "reportGeneralTypeIssues",
		"target": {
			"$mid": 1,
			"path": "/microsoft/pyright/blob/main/docs/configuration.md",
			"scheme": "https",
			"authority": "github.com",
			"fragment": "reportGeneralTypeIssues"
		}
	},
	"severity": 8,
	"message": "Cannot assign member \"reset\" for type \"_Wrapped[(...), Unknown, (*args: Unknown, **kwargs: Unknown), Unknown | Any]\"\n  Member \"reset\" is unknown",
	"source": "Pylance",
	"startLineNumber": 79,
	"startColumn": 13,
	"endLineNumber": 79,
	"endColumn": 18
},{
	"resource": "/home/bswck/dev/jaraco.functools/jaraco/functools/__init__.py",
	"owner": "_generated_diagnostic_collection_name_#2",
	"code": {
		"value": "reportGeneralTypeIssues",
		"target": {
			"$mid": 1,
			"external": "https://github.com/microsoft/pyright/blob/main/docs/configuration.md#reportGeneralTypeIssues",
			"path": "/microsoft/pyright/blob/main/docs/configuration.md",
			"scheme": "https",
			"authority": "github.com",
			"fragment": "reportGeneralTypeIssues"
		}
	},
	"severity": 8,
	"message": "\"range\" is not iterable\n  \"__next__\" method not defined on type \"Iterator[int]\"",
	"source": "Pylance",
	"startLineNumber": 371,
	"startColumn": 14,
	"endLineNumber": 371,
	"endColumn": 22
},{
	"resource": "/home/bswck/dev/jaraco.functools/jaraco/functools/__init__.py",
	"owner": "_generated_diagnostic_collection_name_#2",
	"code": {
		"value": "reportGeneralTypeIssues",
		"target": {
			"$mid": 1,
			"path": "/microsoft/pyright/blob/main/docs/configuration.md",
			"scheme": "https",
			"authority": "github.com",
			"fragment": "reportGeneralTypeIssues"
		}
	},
	"severity": 8,
	"message": "\"KeysView[str]\" is not iterable\n  \"__next__\" method not defined on type \"Iterator[_KT_co@KeysView]\"",
	"source": "Pylance",
	"startLineNumber": 476,
	"startColumn": 41,
	"endLineNumber": 476,
	"endColumn": 47
},{
	"resource": "/home/bswck/dev/jaraco.functools/jaraco/functools/__init__.py",
	"owner": "_generated_diagnostic_collection_name_#2",
	"code": {
		"value": "reportGeneralTypeIssues",
		"target": {
			"$mid": 1,
			"external": "https://github.com/microsoft/pyright/blob/main/docs/configuration.md#reportGeneralTypeIssues",
			"path": "/microsoft/pyright/blob/main/docs/configuration.md",
			"scheme": "https",
			"authority": "github.com",
			"fragment": "reportGeneralTypeIssues"
		}
	},
	"severity": 8,
	"message": "Argument of type \"Unknown | None\" cannot be assigned to parameter \"__source\" of type \"str | ReadableBuffer | CodeType\" in function \"eval\"",
	"source": "Pylance",
	"startLineNumber": 563,
	"startColumn": 33,
	"endLineNumber": 563,
	"endColumn": 36
}]

bswck · 2023-08-14T06:28:27Z

So yes, the proper solution is to move functools into its own package.

An alternative that just came to my mind right now: submit the stub to typeshed and leave jaraco.functools as a module.
An example solution like this:

If the stub was submitted directly to typeshed, migrating jaraco.functools to the package form would not be necessary and mypy users would only have to install types-jaraco.functools as the only additional step. Also, changes made to the original jaraco.functools would appear just slight (type annotations removed from the module at all).
I am thinking of it as a general solution for all untyped/partially typed jaraco.* projects.
What is your view on this? To me, this solution seems like the easiest to maintain in the future, including advantages such as the following:

Any stub-related issues will be reported to typeshed directly, not the implementation repos (including this one).
Any fixes made to the stub, not to the implementation, won't require a consequent additional release.

With that approach fixed, after submitting the stub to typeshed, I could actually create a separate PR here that simply removes all type annotations from jaraco/functools.py. And those would be the only changes made to this repo.

jaraco · 2023-08-14T13:07:54Z

What is your view on [submitting the stubs directly to typeshed]?

My instinct is that's a suboptimal approach because it creates too much distance between the typespec and the implementation, allowing them to diverge. In theory, the type spec should be reflected/inferred from the implementation and thus evolve with the implementation. I've always considered typeshed a forward-compatibility layer until a world exists where the underlying libraries supply their own type specs.

That said, I'm not philosophically opposed; it just feels like the wrong fit. If there are examples or documentation where using typeshed is recommended for cases such as these, I'd be less reluctant.

My preference is still to convert the module to a package. There are many other cases where that becomes necessary (such as adding compatibility modules or breaking the package into semantic groupings), so I consider the barrier to promoting a module to a package to be minimal.

bswck · 2023-08-14T13:18:14Z

Sure! Well then, I'll revert the implementation change and resolve the issues reported by the workflow and by pylance. Thanks for your feedback.

bswck · 2023-08-23T12:53:50Z

@jaraco Could you approve the workflow please? I think it might be it!

bswck · 2023-08-24T07:19:41Z

Sorry, my bad! I imported Concatenate and ParamSpec in 3.9, but they were introduced in 3.10. I don't see any other issue that could prevent the checks from passing. Do you mind approving the workflow again please?

jaraco · 2023-08-24T22:04:55Z

This looks great. Thanks for working through the issues and coming up with a valuable, broad improvement.

bswck · 2023-08-25T06:50:38Z

Thanks! More stubs coming soon. 🚀

jaraco · 2023-08-26T17:33:30Z

This change is implicated in jaraco/pip-run#79.

eli-schwartz · 2023-10-09T15:08:13Z

setup.cfg

@@ -18,6 +18,7 @@ include_package_data = true
 python_requires = >=3.8
 install_requires =
 	more_itertools
+	typing_extensions; python_version < "3.11"


This pull request confuses me. The type annotations are added as stubs, which means that they aren't used at runtime at all -- only by e.g. mypy. Why is a runtime dependency added here?

This forces an additional module to be installed when installing jaraco.functools.

I suspect the dependency was necessary when the hints were inline. Feel free to send a PR to remove. If tests pass, it’s good to go

bswck added 3 commits July 6, 2023 05:49

Add type hints

221a22f

Prefer R to T in annotating return types

c8a78a1

Add py.typed marker

c40551c

bswck changed the title ~~Add type hints~~ Added type hints Jul 6, 2023

jaraco added 2 commits August 5, 2023 20:13

Merge branch 'main' into type-hints

74d96fe

⚫ Fade to black.

4e9de89

jaraco requested changes Aug 6, 2023

View reviewed changes

Restore docstring style

c4f5cdb

jaraco reviewed Aug 6, 2023

View reviewed changes

jaraco/functools.py Outdated Show resolved Hide resolved

Linguistic adjustments for comments and docstrings

2b9c886

bswck commented Aug 9, 2023

View reviewed changes

jaraco/functools.py Outdated Show resolved Hide resolved

jaraco/functools.py Outdated Show resolved Hide resolved

jaraco/functools.py Outdated Show resolved Hide resolved

bswck requested a review from jaraco August 9, 2023 00:17

bswck added 3 commits August 14, 2023 08:12

Transform into package

3b2e107

Add stub

0b3538d

Add py.typed

ed20291

Fix package filenames

d8bc661

bswck added 7 commits August 23, 2023 14:27

Revert implementation changes

aee1a3b

Remove docs/conf.py nitpick_ignore

48afccd

Fix type imports

c28628f

Fix compose() type hints

7661d95

Use float type hint for Throttler.max_rate

ca796f6

Make retry() signature variadic again

7520e07

Shrink signatures to one-line

7b5353b

Remove leftover type annotations

3670822

Fix Concatenate and ParamSpec imports

1b82545

jaraco merged commit 9109ada into jaraco:main Aug 24, 2023
15 checks passed

bswck deleted the type-hints branch August 25, 2023 06:18

This was referenced Aug 25, 2023

Increase stacklevel for DeprecationWarning #24

Closed

Module "jaraco" has no attribute "env" jaraco/pip-run#79

Closed

jaraco mentioned this pull request Aug 26, 2023

Module "jaraco" has no attribute "env" (from jaraco import env) python/mypy#15970

Closed

eli-schwartz reviewed Oct 9, 2023

View reviewed changes

This was referenced Nov 3, 2023

Add type annotations jaraco/jaraco.collections#14

Open

Add type hints jaraco/jaraco.classes#9

Closed

bswck changed the title ~~Added type hints~~ Add type hints Nov 13, 2023

bswck mentioned this pull request Nov 15, 2023

Expand type hints across projects jaraco/skeleton#98

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type hints #22

Add type hints #22

bswck commented Jul 6, 2023 •

edited

Loading

jaraco left a comment

jaraco Aug 6, 2023 •

edited

Loading

jaraco commented Aug 7, 2023

bswck left a comment •

edited

Loading

jaraco commented Aug 12, 2023

bswck commented Aug 14, 2023 •

edited

Loading

bswck commented Aug 14, 2023 •

edited

Loading

bswck commented Aug 14, 2023 •

edited

Loading

jaraco commented Aug 14, 2023

bswck commented Aug 14, 2023

bswck commented Aug 23, 2023 •

edited

Loading

bswck commented Aug 24, 2023 •

edited

Loading

jaraco commented Aug 24, 2023

bswck commented Aug 25, 2023

jaraco commented Aug 26, 2023

eli-schwartz Oct 9, 2023

jaraco Oct 11, 2023

eli-schwartz Oct 11, 2023

Add type hints #22

Add type hints #22

Conversation

bswck commented Jul 6, 2023 • edited Loading

jaraco left a comment

Choose a reason for hiding this comment

jaraco Aug 6, 2023 • edited Loading

Choose a reason for hiding this comment

jaraco commented Aug 7, 2023

bswck left a comment • edited Loading

Choose a reason for hiding this comment

jaraco commented Aug 12, 2023

bswck commented Aug 14, 2023 • edited Loading

bswck commented Aug 14, 2023 • edited Loading

bswck commented Aug 14, 2023 • edited Loading

jaraco commented Aug 14, 2023

bswck commented Aug 14, 2023

bswck commented Aug 23, 2023 • edited Loading

bswck commented Aug 24, 2023 • edited Loading

jaraco commented Aug 24, 2023

bswck commented Aug 25, 2023

jaraco commented Aug 26, 2023

eli-schwartz Oct 9, 2023

Choose a reason for hiding this comment

jaraco Oct 11, 2023

Choose a reason for hiding this comment

eli-schwartz Oct 11, 2023

Choose a reason for hiding this comment

bswck commented Jul 6, 2023 •

edited

Loading

jaraco Aug 6, 2023 •

edited

Loading

bswck left a comment •

edited

Loading

bswck commented Aug 14, 2023 •

edited

Loading

bswck commented Aug 14, 2023 •

edited

Loading

bswck commented Aug 14, 2023 •

edited

Loading

bswck commented Aug 23, 2023 •

edited

Loading

bswck commented Aug 24, 2023 •

edited

Loading