You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Nick Dimiduk <nd...@apache.org> on 2021/12/09 23:58:21 UTC
A tweak to our checkstyle configuration
Heya,
I have posted a small change to our checkstyle configuration on
HBASE-26536. This change will relax the whitespace rules regarding the
left-curly-bracket ('{') character. Specifically, I intend this change to
allow short expressions that include a nested scope that fits entirely on
one line. The example I provide is:
if (foo == null) { return null; }
This whitespace style is already present (though I think not in popular
usage) within the codebase. Please take a look and let me know if you have
any concerns about making this change.
Thanks,
Nick
https://issues.apache.org/jira/browse/HBASE-26536
https://github.com/apache/hbase/pull/3913
Re: A tweak to our checkstyle configuration
Posted by "张铎(Duo Zhang)" <pa...@gmail.com>.
So for #1, would you like to take a look at
https://github.com/apache/hbase/pull/4214
:)
Josh Elser <el...@apache.org> 于2022年3月21日周一 04:50写道：
> Going through my inbox...
>
> 1. Great to have tooling which can validate (and fix) code which is not
> currently to style.
> 2. I would prefer a style guide (such as Google's Java Style) which is
> "generally accepted" by the Java industry at large and we can use as-is.
> However, I don't feel strongly on this.
> 3. I have no objections to Nick's original ask to allow one-line if
> blocks on the same line of code with brackets.
> 4. I prefer brackets for one line if/else blocks over no-brackets for
> the same (as Andrew indicates about avoiding dangling if-else blocks),
> but would not -1 a change if the majority felt otherwise.
>
> On 1/16/22 3:01 AM, 张铎(Duo Zhang) wrote:
> > On enforcing the coding standards, I've filed HBASE-26617, to introduce
> the
> > spotless plugin to HBase.
> >
> > We can add 'mvn spotless:check' to our pre commit checks, so we can
> > enforce the coding standards.
> >
> > And 'mvn spotless:apply' will format everything for you.
> >
> > Andrew Purtell <an...@gmail.com> 于2022年1月16日周日 07:39写道：
> >
> >> There are a handful of anti patterns to avoid, like dangling if-elses.
> >> (Always use braces around code blocks!) Otherwise we have been following
> >> the Java basic guidelines with modifications for indent width and
> maximum
> >> line length and I see no pressing reason why this needs to change. Happy
> >> with the status quo. That said I see no reason to reject Nicks’s small
> >> proposed changes. We definitely don’t need to adopt a totally different
> >> style guide in response to a modest proposal. This seems out of
> proportion
> >> to the ask.
> >>
> >> If we are going to change checkstyle rules it would be necessary for the
> >> proposer to provide a linter for the rest of us to use as well as a
> Yetus
> >> precommit phase that implements the checks. Otherwise it would be a half
> >> completed proposal and worse than making no changes. Please also provide
> >> HOWTOs for configuring the IDEA and Eclipse IDEs.
> >>
> >>> On Jan 15, 2022, at 1:07 AM, 张铎 <pa...@gmail.com> wrote:
> >>>
> >>> What about just switching to use google java style?
> >>>
> >>> Nick Dimiduk <nd...@apache.org> 于2022年1月13日周四 03:22写道：
> >>>
> >>>> Hey all.
> >>>>
> >>>> Discussion on the PR has resulted in an impasse of opinion, but also
> >>>> renewed interest in improvements to static analysis in general
> >>>> (HBASE-26617).
> >>>>
> >>>> I think that this kind of code hygiene is very important for the
> >> long-term
> >>>> maintenance of a large project like ours and especially one that
> accepts
> >>>> contributions from a broad audience. I would really appreciate it if
> >> some
> >>>> more folks would chime into these discussions on PRs, or bring your
> >>>> concerns back up to this thread. I'm game to help see the work done,
> >> but we
> >>>> need more voices to participate in defining what is required by the
> >>>> community.
> >>>>
> >>>> Thanks in advance,
> >>>> Nick
> >>>>
> >>>>> On Thu, Dec 9, 2021 at 3:58 PM Nick Dimiduk <nd...@apache.org>
> >> wrote:
> >>>>>
> >>>>> Heya,
> >>>>>
> >>>>> I have posted a small change to our checkstyle configuration on
> >>>>> HBASE-26536. This change will relax the whitespace rules regarding
> the
> >>>>> left-curly-bracket ('{') character. Specifically, I intend this
> change
> >> to
> >>>>> allow short expressions that include a nested scope that fits
> entirely
> >> on
> >>>>> one line. The example I provide is:
> >>>>>
> >>>>> if (foo == null) { return null; }
> >>>>>
> >>>>> This whitespace style is already present (though I think not in
> popular
> >>>>> usage) within the codebase. Please take a look and let me know if you
> >>>> have
> >>>>> any concerns about making this change.
> >>>>>
> >>>>> Thanks,
> >>>>> Nick
> >>>>>
> >>>>> https://issues.apache.org/jira/browse/HBASE-26536
> >>>>> https://github.com/apache/hbase/pull/3913
> >>>>>
> >>>>
> >>
> >
>
Re: A tweak to our checkstyle configuration
Posted by Josh Elser <el...@apache.org>.
Going through my inbox...
1. Great to have tooling which can validate (and fix) code which is not
currently to style.
2. I would prefer a style guide (such as Google's Java Style) which is
"generally accepted" by the Java industry at large and we can use as-is.
However, I don't feel strongly on this.
3. I have no objections to Nick's original ask to allow one-line if
blocks on the same line of code with brackets.
4. I prefer brackets for one line if/else blocks over no-brackets for
the same (as Andrew indicates about avoiding dangling if-else blocks),
but would not -1 a change if the majority felt otherwise.
On 1/16/22 3:01 AM, 张铎(Duo Zhang) wrote:
> On enforcing the coding standards, I've filed HBASE-26617, to introduce the
> spotless plugin to HBase.
>
> We can add 'mvn spotless:check' to our pre commit checks, so we can
> enforce the coding standards.
>
> And 'mvn spotless:apply' will format everything for you.
>
> Andrew Purtell <an...@gmail.com> 于2022年1月16日周日 07:39写道：
>
>> There are a handful of anti patterns to avoid, like dangling if-elses.
>> (Always use braces around code blocks!) Otherwise we have been following
>> the Java basic guidelines with modifications for indent width and maximum
>> line length and I see no pressing reason why this needs to change. Happy
>> with the status quo. That said I see no reason to reject Nicks’s small
>> proposed changes. We definitely don’t need to adopt a totally different
>> style guide in response to a modest proposal. This seems out of proportion
>> to the ask.
>>
>> If we are going to change checkstyle rules it would be necessary for the
>> proposer to provide a linter for the rest of us to use as well as a Yetus
>> precommit phase that implements the checks. Otherwise it would be a half
>> completed proposal and worse than making no changes. Please also provide
>> HOWTOs for configuring the IDEA and Eclipse IDEs.
>>
>>> On Jan 15, 2022, at 1:07 AM, 张铎 <pa...@gmail.com> wrote:
>>>
>>> What about just switching to use google java style?
>>>
>>> Nick Dimiduk <nd...@apache.org> 于2022年1月13日周四 03:22写道：
>>>
>>>> Hey all.
>>>>
>>>> Discussion on the PR has resulted in an impasse of opinion, but also
>>>> renewed interest in improvements to static analysis in general
>>>> (HBASE-26617).
>>>>
>>>> I think that this kind of code hygiene is very important for the
>> long-term
>>>> maintenance of a large project like ours and especially one that accepts
>>>> contributions from a broad audience. I would really appreciate it if
>> some
>>>> more folks would chime into these discussions on PRs, or bring your
>>>> concerns back up to this thread. I'm game to help see the work done,
>> but we
>>>> need more voices to participate in defining what is required by the
>>>> community.
>>>>
>>>> Thanks in advance,
>>>> Nick
>>>>
>>>>> On Thu, Dec 9, 2021 at 3:58 PM Nick Dimiduk <nd...@apache.org>
>> wrote:
>>>>>
>>>>> Heya,
>>>>>
>>>>> I have posted a small change to our checkstyle configuration on
>>>>> HBASE-26536. This change will relax the whitespace rules regarding the
>>>>> left-curly-bracket ('{') character. Specifically, I intend this change
>> to
>>>>> allow short expressions that include a nested scope that fits entirely
>> on
>>>>> one line. The example I provide is:
>>>>>
>>>>> if (foo == null) { return null; }
>>>>>
>>>>> This whitespace style is already present (though I think not in popular
>>>>> usage) within the codebase. Please take a look and let me know if you
>>>> have
>>>>> any concerns about making this change.
>>>>>
>>>>> Thanks,
>>>>> Nick
>>>>>
>>>>> https://issues.apache.org/jira/browse/HBASE-26536
>>>>> https://github.com/apache/hbase/pull/3913
>>>>>
>>>>
>>
>
Re: A tweak to our checkstyle configuration
Posted by "张铎(Duo Zhang)" <pa...@gmail.com>.
On enforcing the coding standards, I've filed HBASE-26617, to introduce the
spotless plugin to HBase.
We can add 'mvn spotless:check' to our pre commit checks, so we can
enforce the coding standards.
And 'mvn spotless:apply' will format everything for you.
Andrew Purtell <an...@gmail.com> 于2022年1月16日周日 07:39写道：
> There are a handful of anti patterns to avoid, like dangling if-elses.
> (Always use braces around code blocks!) Otherwise we have been following
> the Java basic guidelines with modifications for indent width and maximum
> line length and I see no pressing reason why this needs to change. Happy
> with the status quo. That said I see no reason to reject Nicks’s small
> proposed changes. We definitely don’t need to adopt a totally different
> style guide in response to a modest proposal. This seems out of proportion
> to the ask.
>
> If we are going to change checkstyle rules it would be necessary for the
> proposer to provide a linter for the rest of us to use as well as a Yetus
> precommit phase that implements the checks. Otherwise it would be a half
> completed proposal and worse than making no changes. Please also provide
> HOWTOs for configuring the IDEA and Eclipse IDEs.
>
> > On Jan 15, 2022, at 1:07 AM, 张铎 <pa...@gmail.com> wrote:
> >
> > What about just switching to use google java style?
> >
> > Nick Dimiduk <nd...@apache.org> 于2022年1月13日周四 03:22写道：
> >
> >> Hey all.
> >>
> >> Discussion on the PR has resulted in an impasse of opinion, but also
> >> renewed interest in improvements to static analysis in general
> >> (HBASE-26617).
> >>
> >> I think that this kind of code hygiene is very important for the
> long-term
> >> maintenance of a large project like ours and especially one that accepts
> >> contributions from a broad audience. I would really appreciate it if
> some
> >> more folks would chime into these discussions on PRs, or bring your
> >> concerns back up to this thread. I'm game to help see the work done,
> but we
> >> need more voices to participate in defining what is required by the
> >> community.
> >>
> >> Thanks in advance,
> >> Nick
> >>
> >>> On Thu, Dec 9, 2021 at 3:58 PM Nick Dimiduk <nd...@apache.org>
> wrote:
> >>>
> >>> Heya,
> >>>
> >>> I have posted a small change to our checkstyle configuration on
> >>> HBASE-26536. This change will relax the whitespace rules regarding the
> >>> left-curly-bracket ('{') character. Specifically, I intend this change
> to
> >>> allow short expressions that include a nested scope that fits entirely
> on
> >>> one line. The example I provide is:
> >>>
> >>> if (foo == null) { return null; }
> >>>
> >>> This whitespace style is already present (though I think not in popular
> >>> usage) within the codebase. Please take a look and let me know if you
> >> have
> >>> any concerns about making this change.
> >>>
> >>> Thanks,
> >>> Nick
> >>>
> >>> https://issues.apache.org/jira/browse/HBASE-26536
> >>> https://github.com/apache/hbase/pull/3913
> >>>
> >>
>
Re: A tweak to our checkstyle configuration
Posted by Andrew Purtell <an...@gmail.com>.
There are a handful of anti patterns to avoid, like dangling if-elses. (Always use braces around code blocks!) Otherwise we have been following the Java basic guidelines with modifications for indent width and maximum line length and I see no pressing reason why this needs to change. Happy with the status quo. That said I see no reason to reject Nicks’s small proposed changes. We definitely don’t need to adopt a totally different style guide in response to a modest proposal. This seems out of proportion to the ask.
If we are going to change checkstyle rules it would be necessary for the proposer to provide a linter for the rest of us to use as well as a Yetus precommit phase that implements the checks. Otherwise it would be a half completed proposal and worse than making no changes. Please also provide HOWTOs for configuring the IDEA and Eclipse IDEs.
> On Jan 15, 2022, at 1:07 AM, 张铎 <pa...@gmail.com> wrote:
>
> What about just switching to use google java style?
>
> Nick Dimiduk <nd...@apache.org> 于2022年1月13日周四 03:22写道：
>
>> Hey all.
>>
>> Discussion on the PR has resulted in an impasse of opinion, but also
>> renewed interest in improvements to static analysis in general
>> (HBASE-26617).
>>
>> I think that this kind of code hygiene is very important for the long-term
>> maintenance of a large project like ours and especially one that accepts
>> contributions from a broad audience. I would really appreciate it if some
>> more folks would chime into these discussions on PRs, or bring your
>> concerns back up to this thread. I'm game to help see the work done, but we
>> need more voices to participate in defining what is required by the
>> community.
>>
>> Thanks in advance,
>> Nick
>>
>>> On Thu, Dec 9, 2021 at 3:58 PM Nick Dimiduk <nd...@apache.org> wrote:
>>>
>>> Heya,
>>>
>>> I have posted a small change to our checkstyle configuration on
>>> HBASE-26536. This change will relax the whitespace rules regarding the
>>> left-curly-bracket ('{') character. Specifically, I intend this change to
>>> allow short expressions that include a nested scope that fits entirely on
>>> one line. The example I provide is:
>>>
>>> if (foo == null) { return null; }
>>>
>>> This whitespace style is already present (though I think not in popular
>>> usage) within the codebase. Please take a look and let me know if you
>> have
>>> any concerns about making this change.
>>>
>>> Thanks,
>>> Nick
>>>
>>> https://issues.apache.org/jira/browse/HBASE-26536
>>> https://github.com/apache/hbase/pull/3913
>>>
>>
Re: A tweak to our checkstyle configuration
Posted by Sean Busbey <bu...@apache.org>.
I don't feel strongly about the specific proposal that started this thread.
I could see why someone might find it easier to skim the code; I don't feel
it impacts my reading personally. If there are folks with differing
opinions over it; my preference would be that we stick with whatever style
is already more prevalent in the codebase.
I do feel strongly that we need _some_ consistently enforced standards. I
also think we've been paying a sort of tax as a community for a long time
because our existing code base does not currently follow our stated
standards, so it's confusing. Our section on code conventions[1] doesn't
really give any background beyond "common feedback" as to how we came to
our current formatting rules, but it does tell folks that the relevant
starting point is the sun java conventions[2].
IIRC our current style guidelines grew out of our time in the Apache Hadoop
project which also follows the sun java conventions except with 2 spaces
per indent level instead of 4[3]. It's been modified a bit over time as
things didn't work for us. the biggest difference that comes to mind is our
longer max line length. and maybe the import ordering?
I don't think we should adopt some alternate coding standard all at once
without a good summary of how it compares to our current formatting
standards.
I would be in favor of paying the one-time cost of bringing our active
release lines into compliance with our stated code format standards. This
could be done, for example, by module to break up the reviewing.
[1]: https://hbase.apache.org/book.html#common.patch.feedback
[2]:
https://www.oracle.com/java/technologies/javase/codeconventions-contents.html
[3]:
https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute#HowToContribute-MakingChanges
On Sat, Jan 15, 2022 at 3:07 AM 张铎(Duo Zhang) <pa...@gmail.com> wrote:
> What about just switching to use google java style?
>
> Nick Dimiduk <nd...@apache.org> 于2022年1月13日周四 03:22写道：
>
> > Hey all.
> >
> > Discussion on the PR has resulted in an impasse of opinion, but also
> > renewed interest in improvements to static analysis in general
> > (HBASE-26617).
> >
> > I think that this kind of code hygiene is very important for the
> long-term
> > maintenance of a large project like ours and especially one that accepts
> > contributions from a broad audience. I would really appreciate it if some
> > more folks would chime into these discussions on PRs, or bring your
> > concerns back up to this thread. I'm game to help see the work done, but
> we
> > need more voices to participate in defining what is required by the
> > community.
> >
> > Thanks in advance,
> > Nick
> >
> > On Thu, Dec 9, 2021 at 3:58 PM Nick Dimiduk <nd...@apache.org> wrote:
> >
> > > Heya,
> > >
> > > I have posted a small change to our checkstyle configuration on
> > > HBASE-26536. This change will relax the whitespace rules regarding the
> > > left-curly-bracket ('{') character. Specifically, I intend this change
> to
> > > allow short expressions that include a nested scope that fits entirely
> on
> > > one line. The example I provide is:
> > >
> > > if (foo == null) { return null; }
> > >
> > > This whitespace style is already present (though I think not in popular
> > > usage) within the codebase. Please take a look and let me know if you
> > have
> > > any concerns about making this change.
> > >
> > > Thanks,
> > > Nick
> > >
> > > https://issues.apache.org/jira/browse/HBASE-26536
> > > https://github.com/apache/hbase/pull/3913
> > >
> >
>
Re: A tweak to our checkstyle configuration
Posted by "张铎(Duo Zhang)" <pa...@gmail.com>.
What about just switching to use google java style?
Nick Dimiduk <nd...@apache.org> 于2022年1月13日周四 03:22写道：
> Hey all.
>
> Discussion on the PR has resulted in an impasse of opinion, but also
> renewed interest in improvements to static analysis in general
> (HBASE-26617).
>
> I think that this kind of code hygiene is very important for the long-term
> maintenance of a large project like ours and especially one that accepts
> contributions from a broad audience. I would really appreciate it if some
> more folks would chime into these discussions on PRs, or bring your
> concerns back up to this thread. I'm game to help see the work done, but we
> need more voices to participate in defining what is required by the
> community.
>
> Thanks in advance,
> Nick
>
> On Thu, Dec 9, 2021 at 3:58 PM Nick Dimiduk <nd...@apache.org> wrote:
>
> > Heya,
> >
> > I have posted a small change to our checkstyle configuration on
> > HBASE-26536. This change will relax the whitespace rules regarding the
> > left-curly-bracket ('{') character. Specifically, I intend this change to
> > allow short expressions that include a nested scope that fits entirely on
> > one line. The example I provide is:
> >
> > if (foo == null) { return null; }
> >
> > This whitespace style is already present (though I think not in popular
> > usage) within the codebase. Please take a look and let me know if you
> have
> > any concerns about making this change.
> >
> > Thanks,
> > Nick
> >
> > https://issues.apache.org/jira/browse/HBASE-26536
> > https://github.com/apache/hbase/pull/3913
> >
>
Re: A tweak to our checkstyle configuration
Posted by Nick Dimiduk <nd...@apache.org>.
Hey all.
Discussion on the PR has resulted in an impasse of opinion, but also
renewed interest in improvements to static analysis in general
(HBASE-26617).
I think that this kind of code hygiene is very important for the long-term
maintenance of a large project like ours and especially one that accepts
contributions from a broad audience. I would really appreciate it if some
more folks would chime into these discussions on PRs, or bring your
concerns back up to this thread. I'm game to help see the work done, but we
need more voices to participate in defining what is required by the
community.
Thanks in advance,
Nick
On Thu, Dec 9, 2021 at 3:58 PM Nick Dimiduk <nd...@apache.org> wrote:
> Heya,
>
> I have posted a small change to our checkstyle configuration on
> HBASE-26536. This change will relax the whitespace rules regarding the
> left-curly-bracket ('{') character. Specifically, I intend this change to
> allow short expressions that include a nested scope that fits entirely on
> one line. The example I provide is:
>
> if (foo == null) { return null; }
>
> This whitespace style is already present (though I think not in popular
> usage) within the codebase. Please take a look and let me know if you have
> any concerns about making this change.
>
> Thanks,
> Nick
>
> https://issues.apache.org/jira/browse/HBASE-26536
> https://github.com/apache/hbase/pull/3913
>