We can leave it up to developer's discretion limiting it to the method scope. If it were me fixing the if/else I would fix all the if/else in the current method.
On Wed, Oct 21, 2015 at 10:08 AM, Vlad Rozov <[email protected]> wrote: > Up to what nesting level? Any level? Why it is limited to if(), can it be > function scope or class scope? > > Thank you, > > Vlad > > > On 10/21/15 10:04, Pramod Immaneni wrote: > >> Yes >> >> On Wed, Oct 21, 2015 at 10:00 AM, Vlad Rozov <[email protected]> >> wrote: >> >> This is a variation of option #2 with slightly extended scope. My concern >>> with this approach is that it is vague. In the following case >>> >>> if () { >>> if () { >>> -> logical changes here >>> } else { >>> } >>> } >>> else { ==? is it OK to fix code style here? >>> >>> >>> } >>> >>> On 10/21/15 09:44, Pramod Immaneni 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 >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >
