I think this is exactly what we have been doing. The reasons why I think #1 is good is because:
1) When you write new code that breaks the style check, it's not obvious what specific violation the new code introduces because the code base has already some hundred existing violations. I have already suffered from that many times since the stylecheck plugin was put in. 2) It's confusing to newcomers when they see part of the code is in one format and part of the code is in another, and it's displeasing to the eyes, and it does not promote consistency. 3) git blame -w ignores whitespaces, and it looks like IntelliJ does that too after 11.1.3: https://youtrack.jetbrains.com/issue/IDEA-55075. We are just left with whether curly braces are on the same line for "else", etc. But the bottom line is, the meat of the code should still be largely attributed to the proper authors. David On Wed, Oct 21, 2015 at 9:44 AM, Pramod Immaneni <[email protected]> wrote: > I would go with a modification of 3, you could call it 4. Instead of > re-formatting the entire file that is touched, re-format the code that is > in and around the modified code. Leave that determination to developer > discretion. For example if you are making changes to the if block, fix the > else as well or fix that method if the code changes you are making to the > method are substantial. I would not like the entire file to be changed as > that changes attribution without changing the logic and makes it difficult > to find out whom to talk to when trying to inquire about a piece of code. > > Thanks > > On Wed, Oct 21, 2015 at 9:06 AM, Vlad Rozov <[email protected]> > wrote: > > > I filed https://malhar.atlassian.net/browse/APEX-207 to track the issue. > > Please use it as parent for all check style related JIRAs/fixes. Anyone > to > > volunteer to work on #1 approach? > > > > Thank you, > > > > Vlad > > > > > > On 10/21/15 00:28, David Yan wrote: > > > >> +1 for #1. > >> On Oct 20, 2015 11:15 AM, "Vlad Rozov" <[email protected]> wrote: > >> > >> All, > >>> > >>> We have a large number of existing checkstyle violations and it is > >>> cumbersome to distinguish a newly introduced violation from existing > >>> ones. > >>> We need to agree on the process to fix them and there are multiple > >>> approaches how we can do it. > >>> > >>> 1. Fix them all in a single commit (one commit for Core and one for > >>> Malhar). Pros: change can easily be distinguished from logical code > >>> changes. Cons: large number of changes in a single commit, hard to > >>> review. > >>> Changes and review likely to be done by developers not familiar with > code > >>> specifics. > >>> 2. Fix as we go. Only change code style violation in modified places. > >>> Pros: limited amount of change. Easy to review. Cons: likely to take > >>> forever. Some part of the code may not be fixed at all. > >>> 3. Somewhat combination of 1 & 2. Fix all violations in files affected > by > >>> a commit. Pros: changes likely to be done by developers familiar with > the > >>> code. Cons: harder to distinguish between logical and style changes in > a > >>> single commit. > >>> 4. Any other suggestions? > >>> > >>> Independently of the approach selected, we should not allow commits > where > >>> entire file is modified due to style modifications. Such file(s) needs > to > >>> be fixed and committed using Malhar CI. > >>> > >>> Thank you, > >>> > >>> Vlad > >>> > >>> > >>> > > >
