mandatory code formatting complicates code reviews #200

New issue

Closed

opened 2018-09-03 18:44:04 +02:00 by BrannonKing · 4 comments

BrannonKing commented

2018-09-03 18:44:04 +02:00

(Migrated from github.com)

The code formatting script makes the author's exact changes ambiguous, leading to more complicated code reviews. Reviews would be easier if PRs came in without additional formatting changes.

Options:

Don't require the formatting for the build.
Have the person doing the merge run the formatting tool as part of that step.
Rely upon code review only to catch strange formatting.

We don't currently run code formatting tools on Windows. In addition, the code formatting tools on OSX have proven to have some compatibility differences. Testing and developing on OSX can also be problematic if no one on the team is actively using that OS.

The code formatting script makes the author's exact changes ambiguous, leading to more complicated code reviews. Reviews would be easier if PRs came in without additional formatting changes. Options: * Don't require the formatting for the build. * Have the person doing the merge run the formatting tool as part of that step. * Rely upon code review only to catch strange formatting. We don't currently run code formatting tools on Windows. In addition, the code formatting tools on OSX have proven to have some compatibility differences. Testing and developing on OSX can also be problematic if no one on the team is actively using that OS.

lbrynaut commented

2018-09-04 13:32:23 +02:00

(Migrated from github.com)

If we're not going to use this model, I'd like to see (after the upstream merge) some kind of automated formatter that just runs clang-format nightly across the entire code base and commits changes if any.

The advantage to how it is now is that it guarantees reviewers don't have to page through useless style changes (made manually or automatically by some editors). I suspect removing this will further complicate the review process.

In this model, It's up to the PR author to run formatting to ensure it passes the diff check before issuing it. Maybe that isn't the right answer, but that's what the current checking is trying to enforce. If we're not going to use this model, I'd like to see (after the upstream merge) some kind of automated formatter that just runs clang-format nightly across the entire code base and commits changes if any. The advantage to how it is now is that it guarantees reviewers don't have to page through useless style changes (made manually or automatically by some editors). I suspect removing this will further complicate the review process.

bvbfan commented

2018-09-04 13:47:13 +02:00

(Migrated from github.com)

If we're not going to use this model, I'd like to see (after the upstream merge) some kind of automated formatter that just runs clang-format nightly across the entire code base and commits changes if any.

It looks good to me.

The advantage to how it is now is that it guarantees reviewers don't have to page through useless style changes (made manually or automatically by some editors).

This is exactly how it should be, about me. I mean these is no guaranteed, just ask author to remove them in RR.

> If we're not going to use this model, I'd like to see (after the upstream merge) some kind of automated formatter that just runs clang-format nightly across the entire code base and commits changes if any. It looks good to me. > The advantage to how it is now is that it guarantees reviewers don't have to page through useless style changes (made manually or automatically by some editors). This is exactly how it should be, about me. I mean these is no guaranteed, just ask author to remove them in RR.

kaykurokawa commented

2018-09-05 19:36:32 +02:00

(Migrated from github.com)

Don't require the formatting for the build.

I think this is fine. Because as we've discussed , many areas of the code do not conform to standard formatting and it creates very inconsistent looking code if the PR is a small change. So maybe it is best to ignore formatting issues returned by the clang auto format tool in some cases. And yes its also annoying to have to review formatting changes together with the actual change.

But I still think we should run it anyways individually, and have it show up on travis so that we have some way of enforcing formatting where we need to. Just that we can choose to ignore it in certain situations.

Have the person doing the merge run the formatting tool as part of that step.

I think this could be fine, or there could be seperate PR's just to correct the formatting. Or maybe it can be something automated? If its automated, I think it should not run on the entire code base, maybe just for the claimtrie code or just for any new commits.

>Don't require the formatting for the build. I think this is fine. Because as we've discussed , many areas of the code do not conform to standard formatting and it creates very inconsistent looking code if the PR is a small change. So maybe it is best to ignore formatting issues returned by the clang auto format tool in some cases. And yes its also annoying to have to review formatting changes together with the actual change. But I still think we should run it anyways individually, and have it show up on travis so that we have some way of enforcing formatting where we need to. Just that we can choose to ignore it in certain situations. >Have the person doing the merge run the formatting tool as part of that step. I think this could be fine, or there could be seperate PR's just to correct the formatting. Or maybe it can be something automated? If its automated, I think it should not run on the entire code base, maybe just for the claimtrie code or just for any new commits.

BrannonKing commented

2019-10-29 21:21:58 +01:00

(Migrated from github.com)

It's no longer part of the build.

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbrycrd#200

No description provided.