(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 >>>>>> >>>>>> >>>>>> >>>>>> >
