Skip to content

[TASK] Use native void return type declarations #805

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

Merged
merged 2 commits into from
Jan 25, 2025
Merged

Conversation

oliverklee
Copy link
Collaborator

Also include previous changes for adding native type declarations or strict mode into the changelog.

Also include previous changes for adding native type
declarations or strict mode into the changelog.
@oliverklee
Copy link
Collaborator Author

This will probably clash with #804.

@coveralls
Copy link

coveralls commented Jan 25, 2025

Coverage Status

coverage: 42.138%. remained the same
when pulling 1c5a672 on cleanup/return-void
into 24f44c2 on main.

Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

LGTM. Yes it does clash with #804.

*/
protected function allDeclarationBlocks(array &$aResult)
protected function allDeclarationBlocks(array &$aResult): void
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't help thinking that these all... methods should perhaps be returning the result, rather than having it as a parameter passed by reference - unless the array is supposed to be merged with some original array - but that is beyond the scope of this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looked at the code, and these functions call each other to build up the array, so it does need to be a parameter passed by reference.

@JakeQZ
Copy link
Collaborator

JakeQZ commented Jan 25, 2025

Yes it does clash with #804.

I used the visual merge, which seems to have worked OK.

@JakeQZ JakeQZ merged commit 129f7ce into main Jan 25, 2025
21 checks passed
@JakeQZ JakeQZ deleted the cleanup/return-void branch January 25, 2025 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants