Add type hinting for `open_url` by sivel · Pull Request #85845 · ansible/ansible

sivel · 2025-09-11T17:14:56Z

SUMMARY

This PR adds type hints for open_url and associated functions.

ISSUE TYPE

Test Pull Request

Shrews · 2025-09-16T14:50:55Z

+        ciphers: list[str] | None = None,
+        use_netrc: bool | None = True,
+        context: ssl.SSLContext | None = None,
+    ) -> None:


Is it intentional to allow the params with defaults (use_proxy, force, timeout, etc) to allow None here? That would seem to nullify the effect of calling _fallback() to set the values in open(), no?

None of the default values have changed, they should all be matching the git removal above this.

It wasn't so much about the defaults changing, but more about being explicit that None is allowed. I didn't check any usages to see if we actually do that. It may not actually be a concern, just pointing it out.

Pretty sure it's wrong for args with boolean defaults to have | None.

Shrews · 2025-09-16T14:58:26Z

-            ssl_handler = urllib.request.HTTPSHandler(context=context)
-        handlers.append(ssl_handler)
+        if HAS_SSL:
+            if unix_socket and UnixHTTPSHandler:


If adding the HAS_SSL check, might as well simplify to:

if HAS_SSL and unix_socket and UnixHTTPSHandler:

I've removed that branch. I'm not sure what I was stuck up on before, but I don't seem to be getting a mypy error any longer.

webknjaz · 2025-09-17T18:14:57Z

        ) from e

-    return signature
+    return signature.decode('utf-8')


Perhaps, add type annotations to this function signature before making runtime changes? It's not being type-checked unless you do so. Let MyPy show where your change may possibly cause problems.

It has type annotations, which is how I knew it caused an issue. It was that open_url didn't so it had no inference about what was being returned.

Oh, I have a feeling that sometimes comment-based annotations don't work well. Have you checked it by introducing an intentional error?
This is what I'm worried about:

def get_signature_from_source(source, display=None): # type: (str, t.Optional[Display]) -> str

webknjaz · 2025-09-17T18:18:44Z

+        use_proxy: bool | None = True,
+        force: bool | None = False,
+        timeout: int | None = 10,
+        validate_certs: bool | None = True,
+        url_username: str | None = None,
+        url_password: str | None = None,
+        http_agent: str | None = None,
+        force_basic_auth: bool | None = False,
+        follow_redirects: _FollowRedirects | None = 'urllib2',
+        client_cert: str | None = None,
+        client_key: str | None = None,
+        cookies: cookiejar.CookieJar | None = None,
+        unix_socket: str | None = None,
+        ca_path: str | None = None,
+        unredirected_headers: list[str] | None = None,
+        decompress: bool | None = True,
+        ciphers: list[str] | None = None,
+        use_netrc: bool | None = True,


Usually, bool values mustn't have a union w/ None (except for when it's used as a sentinel or has defined semantics).

Suggested change

use_proxy: bool | None = True,

force: bool | None = False,

timeout: int | None = 10,

validate_certs: bool | None = True,

url_username: str | None = None,

url_password: str | None = None,

http_agent: str | None = None,

force_basic_auth: bool | None = False,

follow_redirects: _FollowRedirects | None = 'urllib2',

client_cert: str | None = None,

client_key: str | None = None,

cookies: cookiejar.CookieJar | None = None,

unix_socket: str | None = None,

ca_path: str | None = None,

unredirected_headers: list[str] | None = None,

decompress: bool | None = True,

ciphers: list[str] | None = None,

use_netrc: bool | None = True,

use_proxy: bool = True,

force: bool = False,

timeout: int | None = 10,

validate_certs: bool = True,

url_username: str | None = None,

url_password: str | None = None,

http_agent: str | None = None,

force_basic_auth: bool = False,

follow_redirects: _FollowRedirects | None = 'urllib2',

client_cert: str | None = None,

client_key: str | None = None,

cookies: cookiejar.CookieJar | None = None,

unix_socket: str | None = None,

ca_path: str | None = None,

unredirected_headers: list[str] | None = None,

decompress: bool = True,

ciphers: list[str] | None = None,

use_netrc: bool = True,

webknjaz · 2025-09-17T18:20:04Z

+        client_cert: str | None = None,
+        client_key: str | None = None,
+        cookies: cookiejar.CookieJar | None = None,
+        use_gssapi: bool | None = False,


Suggested change

use_gssapi: bool | None = False,

use_gssapi: bool = False,

webknjaz · 2025-09-17T18:21:23Z

+    use_proxy: bool | None = True,
+    force: bool | None = False,
+    last_mod_time: _datetime.datetime | None = None,
+    timeout: int | None = 10,
+    validate_certs: bool | None = True,
+    url_username: str | None = None,
+    url_password: str | None = None,
+    http_agent: str | None = None,
+    force_basic_auth: bool | None = False,
+    follow_redirects: _FollowRedirects | None = 'urllib2',
+    client_cert: str | None = None,
+    client_key: str | None = None,
+    cookies: cookiejar.CookieJar | None = None,
+    use_gssapi: bool | None = False,
+    unix_socket: str | None = None,
+    ca_path: str | None = None,
+    unredirected_headers: list[str] | None = None,
+    decompress: bool | None = True,
+    ciphers: list[str] | None = None,
+    use_netrc: bool | None = True,


webknjaz · 2025-09-18T12:56:57Z

+    use_gssapi: bool | None,
+    force_basic_auth: bool | None,
+    use_netrc: bool | None


Can these really be None in runtime?

ansibot added test This PR relates to tests. needs_triage Needs a first human triage before being processed. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Sep 11, 2025

sivel marked this pull request as draft September 11, 2025 18:04

sivel force-pushed the open-url-typing branch from b0a9cac to 3c7fd1f Compare September 11, 2025 18:17

Add type hinting for open_url

1bbf754

sivel force-pushed the open-url-typing branch from 3c7fd1f to 1bbf754 Compare September 11, 2025 18:21

ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Sep 11, 2025

sivel marked this pull request as ready for review September 11, 2025 20:43

Shrews reviewed Sep 16, 2025

View reviewed changes

Remove unnecessary additional branch

94a9698

webknjaz reviewed Sep 17, 2025

View reviewed changes

webknjaz reviewed Sep 18, 2025

View reviewed changes

mkrizek removed the needs_triage Needs a first human triage before being processed. label Sep 23, 2025

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Sep 30, 2025

ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label May 5, 2026

ansibot added the stale_pr This PR has not been pushed to for more than one year. label Sep 21, 2026

Navigation Menu

Repository navigation

Add type hinting for `open_url` - #85845

Add type hinting for `open_url`#85845
sivel wants to merge 2 commits into
ansible:develfrom
sivel:open-url-typing

sivel commented Sep 11, 2025

Uh oh!

Shrews Sep 16, 2025

Uh oh!

sivel Sep 17, 2025

Uh oh!

Shrews Sep 17, 2025

Uh oh!

webknjaz Sep 17, 2025

Uh oh!

Shrews Sep 16, 2025

Uh oh!

sivel Sep 17, 2025

Uh oh!

webknjaz Sep 17, 2025

Uh oh!

sivel Sep 17, 2025

Uh oh!

webknjaz Sep 18, 2025

Uh oh!

webknjaz Sep 17, 2025

Uh oh!

webknjaz Sep 17, 2025

Uh oh!

webknjaz Sep 17, 2025

Uh oh!

webknjaz Sep 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

sivel commented Sep 11, 2025

SUMMARY

ISSUE TYPE

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants