I think (b) would not be as intimidating as it sounds. It's a one-time cost. The change is big but the they are straight forward and should take maybe a couple hours to review but not days. The benefit outweighs the cost IMO.
On Wed, Oct 21, 2015 at 10:28 AM, Munagala Ramanath <[email protected]> wrote: > (a) is fixable :-) but only next month for me. > > Ram > > On Wed, Oct 21, 2015 at 10:21 AM, Vlad Rozov <[email protected]> > wrote: > > > There is no question that we need to fix checkstyle violations and all 3 > > points equally apply to other approaches as well as we work towards > > reducing number of violations and do not allow new ones. To mitigate the > > current large number of violations I suggest to install checkstyle > plugin. > > It is available for all major IDEs and I guess that number of developers > > using vi/emacs is limited :-) > > > > My concern with approach #1 is not that much about loosing git blame > > (annotation in IntelliJ) information, but > > > > a) finding volunteer(s) > > b) once changes are ready, how to pull them in. There is a good chance of > > introduced conflicts while pull request is under review and because of > > massive changes it will take time to review the commit. > > > > Thank you, > > > > Vlad > > > > > > On 10/21/15 10:00, David Yan 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 > >>>>>> > >>>>>> > >>>>>> > >>>>>> > > >
