apex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pramod Immaneni <pra...@datatorrent.com>
Subject Re: Existing checkstyle violations
Date Wed, 21 Oct 2015 17:04:40 GMT
Yes

On Wed, Oct 21, 2015 at 10:00 AM, Vlad Rozov <v.rozov@datatorrent.com>
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 <v.rozov@datatorrent.com>
>> 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" <v.rozov@datatorrent.com> 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
>>>>>
>>>>>
>>>>>
>>>>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message