Skip to content

Change Mapping to dict in resolution Result type annotations - #193

Merged
notatallshaw merged 1 commit into
sarugaku:mainfrom
webknjaz:patch-3
Oct 11, 2025
Merged

Change Mapping to dict in resolution Result type annotations#193
notatallshaw merged 1 commit into
sarugaku:mainfrom
webknjaz:patch-3

Conversation

@webknjaz

@webknjaz webknjaz commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

typing.Mapping is a broad concept. The actual code just assigns dicts so Result should just use that.

I've been adding annotations to the calling side (ansible-galaxy) that uses resolvelib and hit this as I was trying to show that a function that returns Resolver(...).resolve(...).mapping produces a dict.

`typing.Mapping` is a broad concept. The actual code just assigns `dict`s so `Result` should just use that.

I've been adding annotations to the calling side (`ansible-galaxy`) that uses `resolvelib` and hit this as I was trying to show that a function that returns `Resolver(...).resolve(...).mapping` produces a `dict`.
@notatallshaw

Copy link
Copy Markdown
Collaborator

Is it not expected that anyone is sub-classing AbstractResolver to implement their own Resolver? I don't think pip is, but if anyone else is this technically changes the API for resolvelib.

Could this be done at the `Resolver(...).resolve(...) level instead? Or does that break some type theory invariant?

I'm not against this, just thinking about where things could go wrong.

@webknjaz

webknjaz commented Oct 1, 2025

Copy link
Copy Markdown
Contributor Author

@notatallshaw AFAIK, only the AbstractProvider needs to be inherited. Also, if somebody is substituting entire main components of a library they are effectively not using the library in the end.

This cannot be done on the Resolver(...).resolve(...) because it returns a Result object which has a broadly typed attribute. Currently, I'm fighting this with type narrowing: https://github.com/ansible/ansible/pull/85545/files#r2394287530. But I think the types should be fixed here. After all, other components in the repo use dict. So this seems inconsistent.

@notatallshaw

Copy link
Copy Markdown
Collaborator

Great, let's do this.

@notatallshaw
notatallshaw merged commit 260e8e5 into sarugaku:main Oct 11, 2025
7 checks passed
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.

3 participants