Mark Thomas <ma...@apache.org> 于2023年9月5日周二 21:17写道：
> 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 > +1. Me too. > > 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. > I have no objection to a PR that change the coding style like this. 1. There are too many of these old code style in Tomcat, I don't like them either ;), If we accept the PR that change only a little line like this, one line, then the number of PR are huge. To me, that's not contributing. That's making noise. 2. From my perspective as a new contributor, this type of PR is indeed very friendly to new contributors and makes it easy to gain a sense of participation. But I think that if I really wanted to contribute code like this, I wouldn't be able to contribute just one line, because if I really looked at the code, it would be easy to see that there's a lot more that needs to be changed, especially code style. In summary, I think it is necessary for us to create a PR template on GitHub, which is more helpful to help new contributors how to submit PRs correctly. > > 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. > +1. Indeed, the way I handled the previous pr was not reasonable. > 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. > +1 Han > > Mark > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > > -- Han Li
