On 05/09/2023 13:14, Rémy Maucherat wrote:
On Tue, Sep 5, 2023 at 1:48 PM Han Li <aooo...@gmail.com> wrote:
Mark,
I'm not a big fan of this type of PR, this kind of change in the form of a
code change and only a line or two, and I've noticed that the last few PRs
have come from the same place 'woowacourse', which I personally suspect is
what they need to accomplish, eg: task, and since tomcat has a long history
of problems with this form of code, if we allow this kind of change in the
form of a couple of lines of code PR submissions, then I I'm sure there
will be many more of these pointless PRs to show, so I still feel the need
to reject those PRs.
+1, as close to zero benefit as it gets, and lots of noise.
Actually, it is not that far fetched that a large bunch of PRs like
this could be used as an attempt to sneak in a harmful change.
I'm looking at this purely from a Tomcat perspective. If some students
are gaming some course to achieve some assessment objective then that is
an issue for the course to deal with.
I viewed the optimization improvements as worthwhile. I'm generally in
favour of anything that reduces code volume and/or improves readability.
I take the point that the bracket changes are very trivial but given that:
a) I had spent the time reviewing them and confirming they were valid
and
b) I preferred those instances without the extra brackets
it seemed a waste of effort not to merge the PR.
Note: There are a few cases where I think unnecessary brackets make the
code easier to read. For example, I know not everyone can read:
a || b && c
and just know that what order things get processed in.
The point about sneaking in a malicious change is valid concern. It
would certainly be easier to sneak in with a larger volume of changes -
whether that is one big PR or lots of little PRs.
I do worry that rejecting PRs like this - given we have relatively few
new contributors - risks alienating potential future committers.
If we are going to reject PRs like this then the rejection message needs
to be much clearer about why we are rejecting them and to steer them
towards simple PRs that we would accept.
I think one way to address a lot of these concerns would be to enable
UnnecessaryParenthesesCheck, fix all the issues we want to fix and
disable the check for the ones we don't. I'll take a look at this this
afternoon.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
