Skip to content

[Workflow] Added parameter type hinting where possible. #35434

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

Closed
wants to merge 2 commits into from

Conversation

fre5h
Copy link
Contributor

@fre5h fre5h commented Jan 22, 2020

Q A
Branch? master
Bug fix? no
New feature? no
Deprecations? no
Tickets Related to #32223
License MIT
Doc PR no

According to #32223 were added type hintings in most classes, but not in all possible. I tried to add more type hintings. Most of my changes look reasonable, but I am not sure in every.

If you find, that in some class/method type hintings are not welcome, then notice it and I will remove it.
If you find, that all my changes are senseless, then just close this pull request :)

Copy link
Contributor

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

i thought we did do return types automatically ... #33236 or why did we miss those initially?

@lyrixx
Copy link
Member

lyrixx commented Jan 22, 2020

why did we miss those initially?

I don't think so.

Thanks @fre5h for your PR but Most of your change break the BC promise

refs:

@lyrixx
Copy link
Member

lyrixx commented Jan 22, 2020

About the :void on private method (the only changes that does not break BC), it does not bring anything useful.

I'm sorry, But I'm gonna close your PR because It cannot be merged.

@lyrixx lyrixx closed this Jan 22, 2020
@fre5h fre5h deleted the workflow-type-hinting branch January 22, 2020 13:14
@ro0NL
Copy link
Contributor

ro0NL commented Jan 22, 2020

@lyrixx so we missed those because they break BC :) i see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants