Type-annotate `ansible.galaxy.collection` by webknjaz · Pull Request #85961 · ansible/ansible

webknjaz · 2025-10-07T20:37:47Z

This patch is a result of pyrefly autotype with a lot of post-processing.

ISSUE TYPE

Test Pull Request

s-hertel · 2025-10-09T23:37:51Z

 @dataclass
 class ManifestControl:
-    directives: list[str] = None
+    directives: list[str]


This seems like a user-facing change, shouldn't it remain optional?

Suggested change

directives: list[str]

directives: list[str] | None = None

This class is only used to pass things between calls internally, though. But I get your point. MyPy wanted me to do something where it's initialized, I think. I'll double-check if I can revert this bit.

s-hertel · 2025-10-09T23:37:59Z

-    def __init__(self, reasons=None, stdout=None, rc=None, ignore=False):
+    def __init__(
+        self,
+        reasons: t.Iterable | None = None,


Suggested change

reasons: t.Iterable | None = None,

reasons: t.Iterable[str] | None = None,

s-hertel · 2025-10-09T23:39:13Z

+        t.cast(list[str], collection_meta.get('build_ignore', [])),
+        t.cast(
+            ManifestMetadataType | t.Type[Sentinel],
+            collection_meta.get('manifest', Sentinel),
+        ),


_normalize_galaxy_yml_manifest populates the default value for the optional keys (so it contains both 'manifest' and 'build_ignore'). Can we fix the type hint to match, instead of using collection_meta.get()?

I can try. MyPy was mad at me when I was trying to fix it w/o a cast (it thought that the _build_files_manifest() call gets an object which I couldn't explain). I'd prefer that but couldn't make it work for hours. I'd be happy to get rid of the casts if we find a way.

s-hertel · 2025-10-09T23:39:45Z

-    if manifest_control is None:
-        manifest_control = {}
-
    try:
-        control = ManifestControl(**manifest_control)
+        control = ManifestControl(**(manifest_control or {'directives': []}))


This works if manifest_control is None or empty, but if it's truthy and doesn't contain 'directives', it will fail since ManifestControl now requires directives.

@s-hertel it's required in the typing: https://github.com/ansible/ansible/pull/85961/files#diff-4b4657f16ad17de4ea445f2ecec926b0aa800a6065283dcb07eb88491cb94d83R34. Wouldn't that be enough?

It's a change in behavior. I get a crash now when I using a manifest containing the option other than directives.

s-hertel · 2025-10-09T23:40:17Z

-    collection_manifest['file_manifest_file']['chksum_sha256'] = secure_hash_s(files_manifest_json, hash_func=sha256)
-    collection_manifest_json = to_bytes(json.dumps(collection_manifest, indent=True), errors='surrogate_or_strict')
+    collection_manifest_with_hash = {
+        **collection_manifest,
+        'file_manifest_file': {
+            **collection_manifest['file_manifest_file'],
+            'chksum_sha256': secure_hash_s(
+                files_manifest_json,
+                hash_func=sha256,
+            ),
+        },
+    }
+    collection_manifest_json = to_bytes(
+        json.dumps(collection_manifest_with_hash, indent=True),
+        errors='surrogate_or_strict',
+    )


I think the original is easier to read, especially the first line versus the replacement.

@s-hertel yes, but we can't mutate read-only fields. I was trying to make the data structs immutable. Perhaps, we need structs with and w/o the check sum computed. Dunno how to best handle this.

s-hertel · 2025-10-09T23:43:01Z

+    def _get_direct_collection_namespace(
+        self,
+        collection: Collection,
+    ) -> str | None:


Suggested change

) -> str | None:

) -> str:

s-hertel · 2025-10-09T23:43:04Z

+    def _get_direct_collection_name(
+        self,
+        collection: Collection,
+    ) -> str | None:


Suggested change

) -> str | None:

) -> str:

get_direct_collection_meta() sets name+namespace to None in a few cases and so this can just represent that. Otherwise, MyPy will be unhappy about non-matching typing declarations.

If these return None, the only caller will crash with a TypeError.

s-hertel · 2025-10-09T23:43:29Z

-        # type: (t.Union[Candidate, Requirement]) -> dict[str, str]
+    def get_direct_collection_dependencies(
+        self,
+        collection: Candidate | Requirement,


Can this also use Collection, for consistency?

Suggested change

collection: Candidate | Requirement,

collection: Collection,

Maybe, I think in some places I just got the comments converted but in some places it was problematic. I'll check locally, thanks!

s-hertel · 2025-10-09T23:43:45Z

-    sentinel_keys = set()  # type: set[str]
+        galaxy_yml: dict[
+            str,
+            str | list[str] | dict[str, str] | None | t.Type[Sentinel],


Yeah, I temporarily gave up on this function as it's non-trivial to address (hence the cast at the end). It should definitely be refactored more in the future. But I didn't want to make the PR scope too big. I'll accept this suggestion but it'll only cover a little bit of what needs to be improved.

s-hertel · 2025-10-09T23:43:58Z

-):  # type: (...) -> dict
+        b_path: bytes,
+        filename: str,
+) -> dict:


We don't actually validate the MANIFEST.json is a dictionary before we use it as such.

True. But I also just converted the type from comment to explicit syntax. So I didn't look into it. I was trying not to get into making runtime changes unless absolutely necessary. Do you feel like the PR should make such changes?

I just noticed the bug while reviewing, you don't need to fix it.

webknjaz · 2025-10-10T14:58:52Z

 SIGNATURE_COUNT_RE = r"^(?P<strict>\+)?(?:(?P<count>\d+)|(?P<all>all))$"


+class DisplayThread:


Note: I should try to move this back into the original place now that I've solved the display type with a protocol. Originally, I was going to use this class in the union.

This patch is a result of `pyrefly autotype` with *a lot* of post-processing.

webknjaz · 2025-12-10T17:13:20Z


 if t.TYPE_CHECKING:
    from ansible.utils.display import Display
+    from ._types import DisplayQueueType, DisplayThreadProto


(no idea why I have DisplayQueueType here still; will probably have to drop it before merge)

Suggested change

from ._types import DisplayQueueType, DisplayThreadProto

from ._types import DisplayThreadProto

webknjaz · 2025-12-10T17:15:52Z

(last push is just a rebase on top of the moved-out portions of the initial patch; no new changes)

bcoca · 2025-12-10T17:20:09Z

+DisplayQueueType: _t.TypeAlias = _q.Queue[DisplayQueueItemType]
+
+
+class DisplayThreadProto(_t.Protocol):


does this make sense to have here or should it go to a more general _types file?

It may make sense elsewhere. But most of this patch is trying to figure out bits and pieces within the scope of a subpackage/module. I'd definitely consider moving it in a follow-up, but I wouldn't want to include anything beyond ansible.galaxy.collection in the PR scope — it's too fragile atm.

mattclay

There's a lot going on in this PR. Could you split it up to make it easier to review?

There are quite a few "simple" conversions from type comments to native type annotations. That seems like a good place to start, as those should be easy to review. That would make it easier to review the other, more involved changes (which may also benefit from being split up).

webknjaz · 2026-01-09T17:34:46Z

@mattclay agreed. I've actually done that with 3 PRs before PTO and was thinking about doing it again, just haven't returned to this one yet. It's on my list.

webknjaz requested review from jborean93, mattclay, nitzmahone, s-hertel and sivel October 7, 2025 20:37

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

This comment was marked as outdated.

Sign in to view

webknjaz force-pushed the typing/ansible.galaxy.collection branch 2 times, most recently from a59e959 to dd991a4 Compare October 8, 2025 15:07

webknjaz marked this pull request as draft October 8, 2025 15:17

webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Oct 8, 2025

webknjaz force-pushed the typing/ansible.galaxy.collection branch from dd991a4 to d55b934 Compare October 8, 2025 15:38

ansibot added pending_ci and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. ci_verified Changes made in this PR are causing tests to fail. pending_ci labels Oct 8, 2025

webknjaz marked this pull request as ready for review October 8, 2025 21:29

webknjaz moved this to ⏰ Review reminder needed 👀 in 📅 Procrastinating in public 😵‍💫 Oct 8, 2025

webknjaz added this to 📅 Procrastinating in public 😵‍💫 Oct 8, 2025

s-hertel reviewed Oct 9, 2025

View reviewed changes

webknjaz commented Oct 10, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

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 Oct 17, 2025

s-hertel removed the needs_triage Needs a first human triage before being processed. label Oct 21, 2025

This was referenced Nov 28, 2025

🧪 Replace distlib type ignores with stubs #86264

Merged

🧪Drop PkgReq stub @ ansible.galaxy.collection #86265

Merged

This comment was marked as resolved.

Sign in to view

This was referenced Nov 28, 2025

Fix types @ galaxy.collection.galaxy_api_proxy #86266

Merged

🧪 Fix types @ ansible.galaxy.collection.gpg #86267

Merged

webknjaz moved this from ⏰ Review reminder needed 👀 to 🚧 In progress 🚧 in 📅 Procrastinating in public 😵‍💫 Nov 28, 2025

ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Dec 10, 2025

Type-annotate ansible.galaxy.collection

00eabe3

This patch is a result of `pyrefly autotype` with *a lot* of post-processing.

webknjaz force-pushed the typing/ansible.galaxy.collection branch from d55b934 to 00eabe3 Compare December 10, 2025 17:10

ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Dec 10, 2025

webknjaz commented Dec 10, 2025

View reviewed changes

bcoca reviewed Dec 10, 2025

View reviewed changes

ansibot removed 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 Dec 10, 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 Dec 24, 2025

mattclay reviewed Jan 8, 2026

View reviewed changes

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

	reasons: t.Iterable \| None = None,
	reasons: t.Iterable[str] \| None = None,

	str \| list[str] \| dict[str, str] \| None \| t.Type[Sentinel],
	str \| list[str] \| dict[str, str \| bool] \| None \| t.Type[Sentinel],

		SIGNATURE_COUNT_RE = r"^(?P<strict>\+)?(?:(?P<count>\d+)\|(?P<all>all))$"


		class DisplayThread:

	from ._types import DisplayQueueType, DisplayThreadProto
	from ._types import DisplayThreadProto

		DisplayQueueType: _t.TypeAlias = _q.Queue[DisplayQueueItemType]


		class DisplayThreadProto(_t.Protocol):

Uh oh!

Conversation

webknjaz commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ISSUE TYPE

Uh oh!

This comment was marked as outdated.

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!

s-hertel Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

webknjaz Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Uh oh!

webknjaz commented Dec 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattclay left a comment

Choose a reason for hiding this comment

Uh oh!

webknjaz commented Jan 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

webknjaz commented Oct 7, 2025 •

edited

Loading

s-hertel Oct 10, 2025 •

edited

Loading

webknjaz Oct 10, 2025 •

edited

Loading

webknjaz Dec 10, 2025 •

edited

Loading