Are you sure the majority of the formatting changes are number of whitespaces w.r.t your third point.
On Wed, Oct 21, 2015 at 10:00 AM, David Yan <[email protected]> wrote: > 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 > > >>> > > >>> > > >>> > > > > > >
