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
