ext/pdo: applied fixers to improve test robustness - #23023
Conversation
71c7b5c to
608802f
Compare
| clone $stmt; | ||
| } catch (Throwable $e) { | ||
| echo $e::class, ": ", $e->getMessage(), PHP_EOL; | ||
| echo $e::class, ': ', $e->getMessage(), PHP_EOL; |
There was a problem hiding this comment.
I don't like this. Either normalize the whole line or don't change this line at all. We should not be using PHP_EOL in an output. But in this case, this change seems like a totally unnecessary code churn, so I would drop this change.
There was a problem hiding this comment.
Unlike the line endings this is now standardised everywhere; I am not going to change the tool for that.
There was a problem hiding this comment.
Is the tool part of CI? Will it be checked from now on? Why standardise the quote style at all?
There was a problem hiding this comment.
As mentioned in the main PR,this is all generated. There is no reason for double quotes, so there are none. Same as we unify the commas. As usual for linters/fixers they enforce a standard. If every extension would follow the maintainers preference, we don't need a standard
With the line endings in the other thread I agree with you, it should have been unified. But it makes a bigger diff, and it seems the smallest things need bigger discussion and we cannot find a consensus to just get things done. So that is left out for now, unfortunately. That's why I happily will push a commit to change it to \n.
Respectfully, here we are talking about one line -- both of us are right now spending time on discussing quotes of a single line. If you want to change it back to double quotes, please feel free. But please note that I will not change the fixers, which means it will come up again in the next sweep PR.
In case you lack context that was mentioned in the main PR: this stage of the linter/fixer is a first step, it aims to later do other useful things, and ideally one day ends up in CI. We will see how this goes, because also for that, some people seem to like it more and some less. :)
There was a problem hiding this comment.
You can't just ignore core developer objection and hide it as resolved. It cannot be merged if there is an objection and we don't have any other way than RFC to overwrite it which is not exactly the right thing for this. So I'm afraid this will need to be closed unless it is addressed.
There was a problem hiding this comment.
We really should not be applying linters/fixers to PHP unit test files. It is generally a bad idea to do automated mass changes to test code.
The reason for the pushback is not to nitpick. This change will be logged in Git and it will appear in the git blame later on. It's a pointless change because it doesn't fix anything. I'd understood that if you were changing the line to bring actual benefit and you wanted to apply your personal preference, but suggesting changes to unrelated lines just because a tool made that change for you is setting a bad precedent.
If you used a tool to create the PR, that's fine, but I expect you to submit the PR with the improvements that you think are necessary.
I would strongly push back on adding this to the CI pipeline.
There was a problem hiding this comment.
You can't just ignore core developer objection and hide it as resolved. It cannot be merged if there is an objection and we don't have any other way than RFC to overwrite it which is not exactly the right thing for this.
Sorry about the resolving, blindly applied my workflow; not optimal -- will keep in mind going forward!
To the rest:
I hoped that the objection will be revisited because perhaps they would have agreed that I have a point. If that's not the case then that's fine, of course. I don't intend to be stubborn for no reason; and especially I am not pushing back with the intention to disrespect anyones authority here. As you might see in my PR interactions with Jordi, Ilia, Arnaud, and others I am always very eager to address feedback and do so. However, no one can always agree? And I hope that, even I am not a core developer, I am allowed to bring up my arguments, have opinions, and that they are not outright rejected because I am "just" a contributor and your are maintainers.
You know, I am pretty confused the last days tbh. The issue here is that there are many maintainers; everyone logically has their own preferences. I get that! But what can I do? Keep tweaking and adding complexity and (from my point of view) unnecessary work for myself for what is from my perspective a no-brainer. I see value doing this work, but in some cases the feedback is overly bureaucratic -- tough, of course my feeling can be wrong, no disrespect, just speaking my mind. Allow me to remind that the total amount of changed lines of the main PR was something in the 19,000s. Roughly 350 lines (did the math in the main PR; things likely drifted a bit since) had honest style only changes. The vast majority of it in files that were edited anyway. A huge amount got already merged, with one manual intervention (because of a removed getTrace() in a non-asserted catch; so it was indeed not as clear as it seems here to me. Perhaps I am too new here to understand, but does't the value the work I did here add by far outweigh this small exception?
Look, I am here to help PHP, and support y'all. I am eager to help because I see that y'all have a tonne of work, that people from "outside" always complain but never take action. I want to make a difference, I am trying my best to do so, and I plan to keep doing that. But I also cannot, like, make everyone happy within a task with such a scope, unfortunately. So yeah, seriously a bit of ideas what else I could say.
So I'm afraid this will need to be closed unless it is addressed.
If the two single quotes are an absolute show stopper, and the added value does not outweigh it, then I guess I will have to accept that decision. It's like an RFC, I propose something and if it is rejected I respect that without hard feelings -- and of course I'll will keep making myself useful even if it is closed. 🫡
Of course a maintainer could also just add a code suggestion, I click accept, and then will in the future no longer open hardening PRs to this extension. If that makes sense, I naturally have no issues in doing so.
There was a problem hiding this comment.
We really should not be applying linters/fixers to PHP unit test files.
I respect that opinion, and I am not the one who decides. But allow me to mention that others already encouraged me to do more. So yeah, that's the situation I -- as the one who contributes from the outside -- am in right now. Different maintainers, different opinions. Not sure how I myself can solve that.
It is generally a bad idea to do automated mass changes to test code.
Here I would respectfully push back. Each rewrite is running the actual phpt tests. The linter/fixer I built has extensive fixtures and test coverage. Naturally, we never claim something to be safe -- but that's why I reviewed every single PR diff carfully. I don't see how an automation that helps me to do the job would per se be a bad idea. No one wants to make this kinda work manually, but it adds value, so I came up with a solution. I honestly expected more excitement for it initially -- I will keep learning.
The reason for the pushback is not to nitpick. This change will be logged in Git and it will appear in the git blame later on.
I see the point and don't object.
It's a pointless change because it doesn't fix anything. I'd understood that if you were changing the line to bring actual benefit and you wanted to apply your personal preference, but suggesting changes to unrelated lines just because a tool made that change for you is setting a bad precedent.
If you used a tool to create the PR, that's fine, but I expect you to submit the PR with the improvements that you think are necessary.
Maybe this is where we have a miscommunication? I built the fixer, so logically I did take this decisions actively. Some before, and some after feedback in the main PR. This one was taken before any feedback. The reason why I opted to unify this twofold A) there is no reason for the double quotes because there is no var inside, but most importantly, B) the vast majority in the existing code uses single-quotes: 198 vs 2,604.
So it's not that I blindly did let that happen. I just had good intentions when I decided that this is worth to unify in one sweep. ✌️
I would strongly push back on adding this to the CI pipeline.
That's fair.
I really don't act in bad fait here. I meant all what I offered good and didn't expect these kind of pushback to be honest. As mentioned above, I will learn and adjust.
There was a problem hiding this comment.
Thanks for the PR. I do not object to adding the exception class names to the expectations; I believe that it is a positive improvement. However, tests are meant to be loose and contain differing opinions. If we changed all the tests as we please to match some standard, we could accidentally change the test in a way that it doesn't test the bug/regression it was designed to test. Sometimes, the original author consciously makes a decision to stray from the norm to make the test more exotic and test all possible variations. Sometimes, it is done to add intentional ambiguity. Thus, even a simple change, such as changing single to double quote, should be viewed as potentially important. This is why a test suite is different from the typical source code.
Co-authored-by: Kamil Tekiela <tekiela246@gmail.com>
3 participants
Asserts error class; extracted from: #22799