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

Use t.Mapping not t.Dict for parameters #2265

Merged
merged 5 commits into from May 8, 2023

Conversation

altendky
Copy link
Contributor

@altendky altendky commented Apr 29, 2022

https://docs.python.org/3/library/typing.html#typing.Dict

To annotate arguments it is preferred to use an abstract collection type such as Mapping.

This PR does almost that to fix the issue below. I instead use MutableMapping here since we do mutate in some cases. I chose to use MutableMapping everywhere for consistency and leaving it open for more mutation in the future. I understand that this may not be preferred and would be happy to use Mapping where possible instead.

It was suggested I could follow #2256 as a guide for how much of this checklist applies to hint-only changes. As such some have been skipped.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@altendky altendky marked this pull request as ready for review April 29, 2022 16:50
Copy link
Collaborator

@saroad2 saroad2 left a comment

Choose a reason for hiding this comment

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

Hey @altendky , Thanks for the PR and sorry for the long delay!

This PR makes sense to me. I fixed some merging issues and we are ready to merge.
The only thing that I think worth fine tuning is the change note. I wrote a suggestion, but if you have better one - please feel free to paraphrase.

After this is fixed, we could merge!

CHANGES.rst Outdated Show resolved Hide resolved
@altendky
Copy link
Contributor Author

altendky commented May 7, 2023

Your suggestion reads well to me. Thanks for your interest and effort on this.

@saroad2
Copy link
Collaborator

saroad2 commented May 7, 2023

I'm now rethinking that this PR might should be merged into 8.1.x instead of main.

@davidism , I would appriciate your input on it.

@davidism
Copy link
Member

davidism commented May 8, 2023

Yes, this can go in 8.1.x.

@saroad2 saroad2 changed the base branch from main to 8.1.x May 8, 2023 14:54
@saroad2
Copy link
Collaborator

saroad2 commented May 8, 2023

I changed the target branch and reworked the commits history. This is now ready to be merged.

Thanks @altendky again for the PR. I hope to see more of your contributions and the future.

@saroad2 saroad2 merged commit 8473b3c into pallets:8.1.x May 8, 2023
12 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to properly inherit and hint Group due to use of Dict for parameters
3 participants