-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
[PHPUnitBridge] Improved deprecations display #35271
Conversation
d803a11
to
5a5df3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay, continuous refactoring, a.k.a leaving the place better than before. I do the same. This bridge especially needs it. Few more like this in future should hopefully come.
src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/Configuration.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/DeprecationGroup.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/DeprecationNotice.php
Show resolved
Hide resolved
src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/DeprecationNotice.php
Show resolved
Hide resolved
++$this->count; | ||
} | ||
|
||
public function countsByCaller() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function countsByCaller() | |
public function getMethodOccurences() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method where the deprecation is triggered is not to be confused with the method that calls that method, and countsByCallingMethod
seems a bit long, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, they seem symmetric to me so I would expect similar name. I guess we need 3rd opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getCountsByCaller
?
aafa219
to
dbffb71
Compare
I added per-group verbosity, the usage is |
src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/Configuration.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/partially_quiet.phpt
Outdated
Show resolved
Hide resolved
src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/partially_quiet.phpt
Show resolved
Hide resolved
src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/Configuration.php
Outdated
Show resolved
Hide resolved
[key]=int is more consistent with what we have now |
9970c33
to
8d56650
Compare
8d56650
to
e775a6d
Compare
I just pushed a version with a test case that makes more sense. Also, I added a commit to remove the "remaining" prefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, here are my comments.
Can you please add a line in the changelog of the bridge?
Can you please also update to the description of the PR so that it documents what this is about? I had to review in details to get it, the ppl that will write the doc and/or the blog post about this need help too.
src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/Configuration.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/DeprecationNotice.php
Outdated
Show resolved
Hide resolved
++$this->count; | ||
} | ||
|
||
public function countsByCaller() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getCountsByCaller
?
src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/DeprecationGroupTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/DeprecationNoticeTest.php
Outdated
Show resolved
Hide resolved
@@ -33,7 +33,7 @@ require __DIR__.'/fake_vendor/acme/outdated-lib/outdated_file.php'; | |||
|
|||
?> | |||
--EXPECTF-- | |||
Remaining indirect deprecation notices (1) | |||
Indirect deprecation notices (1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the "remaining" prefix? It made the output more friendly to me, I see no reasons to drop it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was my suggestion, check #35271 (comment). Remaining implies non-remaining ones are displayed somewhere else. Please help us see how it improves friendliness using that word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you're in the process of fixing deprecations - ie when you look at those reports, these are the "remaining deprecations" triggered by your code, until you fix them all. I don't read "remaining" as you and thus I don't see the confusion. Writing "Indirect deprecation notices (1)" feels too... direct. "Remaining" gives a direction to the cold fact, thus helps humans (because human don't know what to do with cold facts, but that's going off topic :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not „Deprecations you can fix“ and „Vendor Deprecations“?
Another wording is fine too, you get my point... hopefully 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there is an issue with the current wording, reducing merge conflicts is my priority, thanks for understanding :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can say the same about normal failing phpunit test cases. They are supposed/waiting to be fixed, but phpunit doesn't say "remainig failures". AFAIK nowhere else than phpunit bridge is this concept used like this.
Also, in practice user can't fix Indirect deprecations, but they are "remaining". So there is nothing to fix for user similar as "legacy" deprecations, but unlike legacy deprecations they are for some reason marked as "remaining".
To me whole remaining concept just makes things more confusing. But since we disagree, I guess this should be extracted to separate PR/issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OskarStark also, "remaining" was also used for vendor deprecations :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"vendor" deprecations are not a thing.
"indirect" are - YOU are always responsible for them, all of them. It could be because YOU decided to use this package or YOU didn't contribute the fix yet, or YOU are using the package in a deprecated way.
Let's stop the wrong narrative about "it's them, not me".
Sorry for not doing that sooner, now that I wrote it guess the usage is a bit confusing, maybe the behavior should be changed, and it people should rather say which groups should be made quiet with |
|
or |
ouch, I'm going again in the other alternative not suggested by @ostrolucky, sorry... |
Yeah, also, only one of |
e775a6d
to
5782a1e
Compare
@nicolas-grekas what should happen if we have |
Nothing special to me. |
d4b9c32
to
f56fc61
Compare
@nicolas-grekas implemented :) |
@nicolas-grekas should I contribute the first commit to a lower branch first? It's a refactoring (and a big one). |
nope, refacto are always for master: reducing the risk of introducing regressions has an even higher priority. |
f56fc61
to
dac6ad7
Compare
Ok, I just dropped the last commit (about "remaining") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments and good to me (I didn't run the PR so I'm counting on you to fix any issues we missed during review ;) )
* whether to display a stack trace | ||
* @param bool $verboseOutput | ||
* @param string $regex Will be matched against messages, | ||
* to decide whether to display a stack trace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unneeded diff
@@ -72,7 +72,15 @@ private function __construct(array $thresholds = [], $regex = '', $verboseOutput | |||
} | |||
} | |||
$this->regex = $regex; | |||
$this->verboseOutput = $verboseOutput; | |||
|
|||
$this->verboseOutput = array_fill_keys(['unsilenced', 'direct', 'indirect', 'self', 'other'], true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking, this still has runtime overhead, whereas declaring the resulting array is free. Also, I doubt this is more readable since it requires knowing what array_fill_keys()
does.
|
||
namespace Symfony\Bridge\PhpUnit\DeprecationErrorHandler; | ||
|
||
final class DeprecationGroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's mark all new classes as @internal
final class DeprecationNotice | ||
{ | ||
/** | ||
* @var int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping
dac6ad7
to
ef62255
Compare
I just tested on SonataAdminBundle, and I couldn't find any issues 😛 |
ef62255
to
c55a89e
Compare
Thank you @greg0ire. |
This PR was merged into the 5.1-dev branch. Discussion ---------- [Typo] Rename occurence to occurrence | Q | A | ------------- | --- | Branch? | master (5.1) | Bug fix? | yes (typo) | New feature? | no | Deprecations? | no | Tickets | Fix #36242 | License | MIT | Doc PR | Not needed This PR will fix correct spelling of methods containing the word occurrence implemented with PR : - [PHPUnitBridge] Improved deprecations display #35271 This changes are on master branch, because the original PR is a new feature for Symfony 5.1. Commits ------- 11f746a [Typo] Rename occurence to occurrence
quiet[]=indirect&quiet[]=direct
Might add more improvements to that branch if people suggest some.