docker-py icon indicating copy to clipboard operation
docker-py copied to clipboard

Add types

Open Viicos opened this issue 1 year ago • 2 comments

Part of #2796.

  • [ ] api (probably not in this PR)
  • [ ] context (same)
  • [ ] credentials (same)
  • [ ] models (same)
  • [ ] transport (same)
  • [ ] types (same)
  • [ ] utils (same)
  • [x] auth.py
  • [x] client.py
  • [x] constants.py
  • [x] errors.py
  • [x] tls.py
  • [x] version.py
  • [ ] Add an entry to MAKEFILE, + items described here

I have used Dict[str, Any] in auth.py and client.py in some places, we might want to narrow the types using TypedDict instead (or other solutions, can be discussed).

Notes:

  • As adding types will add a lot of imports and changes to the code layout, isort and a code formatter could be enforced.

  • I suggest we change json and binary to keyword only arguments. If used as an argument, this could lead to confusion (see here for example). https://github.com/docker/docker-py/blob/82cf559b5a641f53e9035b44b91f829f3b4cca80/docker/api/client.py#L272

  • config_path and config_dict can be made Optional, as this classmethod is already called with optional arguments here. https://github.com/docker/docker-py/blob/82cf559b5a641f53e9035b44b91f829f3b4cca80/docker/auth.py#L153-L164

  • Before continuing, I'd like to know if:

    • Using typing_extensions is fine (required for Literal, introduced in Python3.8)
    • You want to have type stubs defined either in code, in pyi files, or implemented in typeshed. Some methods require overloads, and this would add a lot of boilerplate in the source code itself, wich is may not be what people contributing to this repository want to see. If we chose to implement stubs in separate pyi files, we could type the public API only, but developers of this library won't benefit from types for internal objects.

(@aiordache @ulyssessouza @milas; tagging as this is a draft PR).

Viicos avatar Jan 02 '23 19:01 Viicos

What I don't see in this MR is:

  • something like instructions for how to check what you added with either mypy or pyright
  • a test target for a makefile
  • info in a Readme
  • or literally anything that can help the next person avoid regressions or help them continue your work, respectively.

Could you check to see what conventions (e.g. auto tests, makefile, similar) are used by the project and try to throw together the simplest integration you could think of? Ask for help if you can't figure it out yourself, that's fine :)

GhostLyrics avatar Jul 07 '23 10:07 GhostLyrics

What I don't see in this MR is: [...] Could you check to see what conventions (e.g. auto tests, makefile, similar) are used by the project and try to throw together the simplest integration you could think of? Ask for help if you can't figure it out yourself, that's fine :)

As I said here, I was (and still am) waiting for approval from the maintainers of the project. I don't want to add types directly into the project and then see this PR declined because maintainers doesn't want to use them. That's why we are thinking about adding them to typeshed (see discussion), and I will surely do it if we don't have any feedback from maintainers in the next few days

Viicos avatar Jul 07 '23 11:07 Viicos