8000 [DependencyInjection] Add types to private/final/internal methods and constructors by derrabus · Pull Request #33266 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DependencyInjection] Add types to private/final/internal methods and constructors #33266

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 8, 2019
Merged

[DependencyInjection] Add types to private/final/internal methods and constructors #33266

merged 1 commit into from
Sep 8, 2019

Conversation

derrabus
Copy link
Member
Q A
Branch? 4.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #32179, #33228
License MIT
Doc PR NA/

@derrabus derrabus requested a review from dunglas as a code owner August 20, 2019 22:10
@nicolas-grekas nicolas-grekas added this to the next milestone Aug 21, 2019
@nicolas-grekas nicolas-grekas mentioned this pull request Aug 21, 2019
Merged
nicolas-grekas added a commit that referenced this pull request Aug 21, 2019
@nicolas-grekas
minor #33279 Add return types to tests and final|internal|private met…
55cd8d6 
…hods (nicolas-grekas, derrabus)

This PR was merged into the 4.4 branch.

Discussion
----------

Add return types to tests and final|internal|private methods

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #33228 #33236
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

#33266 made me spot an issue with my logic to add return types with help from `DebugClassLoader`.
Here is a completing PR, with the logic expanded to test classes.

I need help to fix tests, /cc @derrabus :)

Commits
-------

c39fd9c Fixed tests on the Security and Form components
fc186bb Add return types to tests and final|internal|private methods
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Aug 21, 2019 •
edited
Loading

Honestly, I'm not a big fan of adding void. e.g. on test cases, they are pure visual noise.
Can you please rebase this PR, and maybe move the void return types to another PR if you want to push them forward?

@derrabus
Copy link
Member Author

Honestly, I'm not a big fan of adding void. e.g. on test cases

Hey, I could have added void to all test* methods, but I thought that might hurt your eyes a little too much. 😅

As I already said on #33228, I might need a llittle guidance on when to add void. Sometimes you add void yourself, sometimes you state that you don't like void.

@derrabus derrabus force-pushed the improvement/dependency-injection-types branch from c21fbb2 to 767023f Compare August 21, 2019 16:10
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Aug 21, 2019 •
edited
Loading

Sometimes you add void yourself, sometimes you state that you don't like void.

:'D I did add them when a third-party dep encourages us to do so. I may have added more but I regret already :)

sstok
sstok reviewed Aug 28, 2019
View reviewed changes
@derrabus derrabus force-pushed the improvement/dependency-injection-types branch from 767023f to fb74fbf Compare August 30, 2019 16:50
@derrabus
Copy link
Member Author

If you've heard someone weeping, it was probably me reverting all the void declarations. PR is ready now. 😃

@derrabus derrabus force-pushed the improvement/dependency-injection-types branch from fb74fbf to def0ac7 Compare September 8, 2019 19:38
@nicolas-grekas
Copy link
Member

Thank you @derrabus.

nicolas-grekas added a comm 8000 it that referenced this pull request Sep 8, 2019
@nicolas-grekas
minor #33266 [DependencyInjection] Add types to private/final/interna…
200eae3 
…l methods and constructors (derrabus)

This PR was merged into the 4.4 branch.

Discussion
----------

[DependencyInjection] Add types to private/final/internal methods and constructors

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179, #33228
| License       | MIT
| Doc PR        | NA/

Commits
-------

def0ac7 Add types to private/final/internal methods and constructors.
@nicolas-grekas nicolas-grekas merged commit def0ac7 into symfony:4.4 Sep 8, 2019
@derrabus derrabus deleted the improvement/dependency-injection-types branch September 8, 2019 21:36
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sstok sstok sstok left review comments

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

Assignees
No one assigned
Labels
DependencyInjection Status: Needs Review
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
4 participants
@derrabus @nicolas-grekas @sstok @carsonbot
0