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

Add types #3085

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Add types #3085

wants to merge 5 commits into from

Conversation

Viicos
Copy link

@Viicos Viicos commented Jan 2, 2023

Part of #2796.

  • api (probably not in this PR)
  • context (same)
  • credentials (same)
  • models (same)
  • transport (same)
  • types (same)
  • utils (same)
  • auth.py
  • client.py
  • constants.py
  • errors.py
  • tls.py
  • 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).

    def _result(self, response, json=False, binary=False):

  • config_path and config_dict can be made Optional, as this classmethod is already called with optional arguments here.

    docker-py/docker/auth.py

    Lines 153 to 164 in 82cf559

    @classmethod
    def load_config(cls, config_path, config_dict, credstore_env=None):
    """
    Loads authentication data from a Docker configuration file in the given
    root directory or if config_path is passed use given path.
    Lookup priority:
    explicit config_path parameter > DOCKER_CONFIG environment
    variable > ~/.docker/config.json > ~/.dockercfg
    """
    if not config_dict:
    config_file = config.find_config_file(config_path)

  • 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).

docker/api/client.py Outdated Show resolved Hide resolved
@GhostLyrics
Copy link

GhostLyrics commented Jul 7, 2023

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 :)

@Viicos
Copy link
Author

Viicos commented Jul 7, 2023

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

Signed-off-by: Victorien Plot <65306057+Viicos@users.noreply.github.com>
Signed-off-by: Viicos <65306057+Viicos@users.noreply.github.com>
Signed-off-by: Victorien Plot <65306057+Viicos@users.noreply.github.com>
Signed-off-by: Viicos <65306057+Viicos@users.noreply.github.com>
Signed-off-by: Viicos <65306057+Viicos@users.noreply.github.com>
Signed-off-by: Viicos <65306057+Viicos@users.noreply.github.com>
- name: Run ruff
run: ruff docker tests
- name: Run mypy
run: mypy docker

Choose a reason for hiding this comment

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

I'd suggest putting the files in the config and then only running mypy.
I'd also suggest copying, for example, this config, to get more in depth checking: https://github.com/aio-libs/aiohttp-jinja2/blob/master/.mypy.ini
Commenting out any options that you don't want to deal with yet, so they can be added in later (note you can also do config options per module).

Copy link
Author

Choose a reason for hiding this comment

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

Or maybe we should go with mypy strict in one go?

Choose a reason for hiding this comment

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

strict just enables about half the options listed in that config (there are several other useful ones not enabled by strict), so I'd still look at the whole thing. And if you don't want to resolve everything in 1 PR, then more finegrained settings is going to help.

Copy link
Author

Choose a reason for hiding this comment

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

I see, I'll take a look. I'll see if CI passes once I convert this PR as ready to review. But I don't think maintainers will see..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants