aglinxinyuan commented on PR #4342: URL: https://github.com/apache/texera/pull/4342#issuecomment-4228972231
> > We usually make minor fixes like formatting along with major changes in a single PR. > > That's fine, but if you did that, please mention it in PR description so reviewers won't find it surprising. > > > it is possible to make every single line of change a separate PR, and they would still be logically correct, but it would dramatically decrease productivity. > > It is a balance. besides the PR author's effort, we also need to consider the changes you are making would affect others who work on the same file, and also the time reviewers are putting on the PR. so ideally it is always better to minimize change so that the conflicts are small, and reviews are easier. For instance, as a reviewer if I know you are only change format then it's a simple stamp from me. If you mix formatting and logic changes within the same pr, and does not let me know up front, then my review will be full of questions like "why do you change this, this feels unrelated to the PR". which also waste time of the team. > > > Of course, I don’t have to do that, and it doesn’t make much sense to create a separate PR just for that. > > Actually, making another PR for formatting only changes, or comment-only changes, are recommended. they can be marked as minor PRs, and can make review easier. I'm aware of and agree with those principles. It really comes down to balance, which is why I mentioned earlier that it's very subjective. In fact, I already separated the major related formatting changes into three PRs to make the review process easier: #4332, #4334, and #4336. The remaining formatting changes, in my judgment, are too minor to warrant a separate PR or even to be mentioned in the PR description. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
