Skip to content

Add type hints#747

Merged
lafrech merged 4 commits intomarshmallow-code:devfrom
kasium:typing-2
Mar 17, 2022
Merged

Add type hints#747
lafrech merged 4 commits intomarshmallow-code:devfrom
kasium:typing-2

Conversation

@kasium
Copy link
Copy Markdown
Contributor

@kasium kasium commented Feb 4, 2022

Closes #745

@kasium kasium marked this pull request as draft February 4, 2022 13:45
@kasium
Copy link
Copy Markdown
Contributor Author

kasium commented Feb 6, 2022

@lafrech I added as many type hints as possible. Can you please have a look?

Copy link
Copy Markdown
Member

@lafrech lafrech left a comment

Choose a reason for hiding this comment

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

Thank you very much already for the contribution.

I didn't review thoroughly already, but this looks good.

I think we should use

from __future__ import annotations

as in marshmallow-code/marshmallow#1932.

Comment thread setup.cfg Outdated
Comment thread src/apispec/yaml_utils.py Outdated
@kasium
Copy link
Copy Markdown
Contributor Author

kasium commented Feb 9, 2022

Thank you very much already for the contribution.

I didn't review thoroughly already, but this looks good.

I think we should use

from __future__ import annotations

as in marshmallow-code/marshmallow#1932.

good idea. I changed the hints to that system

@kasium kasium requested a review from lafrech February 9, 2022 16:55
@kasium
Copy link
Copy Markdown
Contributor Author

kasium commented Feb 10, 2022

I got this weird CI error: No hosted parallelism has been purchased or granted. To request a free parallelism grant, please fill out the following form https://aka.ms/azpipelines-parallelism-request

@kasium kasium marked this pull request as ready for review February 10, 2022 07:14
@lafrech
Copy link
Copy Markdown
Member

lafrech commented Feb 10, 2022

Strange. We're about to migrate from Azure, anyway. To GH actions + pre-commit.ci.

Please give me a little time to check this. Thanks again.

Copy link
Copy Markdown
Member

@lafrech lafrech left a comment

Choose a reason for hiding this comment

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

Looks pretty good.

Do you think you could squash all your commits and push-force?

This would provide a nicer commit history and it would also trigger a new CI run. Not sure what failed with Azure, let's see how it goes this time.

Thanks.

@kasium kasium requested a review from lafrech February 10, 2022 22:11
@kasium
Copy link
Copy Markdown
Contributor Author

kasium commented Feb 10, 2022

@lafrech the CI still doesn't work. I tried to squash everything but merge commits and rebases are not nice.
However, if you merge the PR, you can normally also select squash.

@lafrech
Copy link
Copy Markdown
Member

lafrech commented Feb 10, 2022

Indeed, I read a bit more about the CI error message. I don't want to struggle with this while about to change CI anyway. I shall move to Github actions sooner than I expected.

@lafrech
Copy link
Copy Markdown
Member

lafrech commented Mar 17, 2022

@kasium sorry about the delay. CI issues should now be fixed.

Do you think you could rebase to let the tests run before we merge?

Thanks !

@kasium
Copy link
Copy Markdown
Contributor Author

kasium commented Mar 17, 2022

@lafrech done

@lafrech lafrech merged commit deb897f into marshmallow-code:dev Mar 17, 2022
@lafrech
Copy link
Copy Markdown
Member

lafrech commented Mar 17, 2022

Thank you so much!

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.

Add typing

3 participants