Hi @Y Ethan Guo,
I am very willing to follow the community's decision. And your idea is very
good, we can disable those checkstyle rules that cannot be automated for
reformatting in IDE for now.
best,
lamber-ken
At 2019-12-24 03:38:55, "Y Ethan Guo" <ethan.guoyi...@gmail.com> wrote:
>+1 on auto-formatting the code in IDE based on the checkstyle rules.
>
>Based on my experience with Java and Scala in IntelliJ, there's is indeed
>discrepancy on auto formatting code on some custom checkstyle rules. For
>such cases, I tried to avoid using them if they do not sacrifice too much
>on the code style. Reformatting code in IDE based on a different rule
>causing checkstyle errors has decreased my productivity before.
>
>So I'm wondering if @lamber-ken is willing to disable those checkstyle
>rules that cannot be automated for reformatting in IDE for now. Once
>spotless or another plugin can solve this issue, we can re-enable those
>rules.
>
>On Mon, Dec 23, 2019 at 11:19 AM nishith agarwal <n3.nas...@gmail.com>
>wrote:
>
>> Vinoth,
>>
>> +1 on automating the manual work required at the moment to fix the
>> checkstyle errors. I think if we are able to use spotless and
>> at the same time know upfront all the things that would require manual
>> work, there are few options IMO :
>>
>> a) Have a template of steps that can easily fix it -> for eg. selecting a
>> specific file, forcing checkstyle corrections through some intellij
>> sequence of steps. (I have noticed sometimes selecting certain parts of the
>> code and reformat manually helped, may be my intellij wasn't setup
>> correctly at the time). This documented, repeatable process can slightly
>> reduce the time taken to fix them.
>> b) Find a different tool that can address these shortcomings
>> c) Relax some of the checkstyles that are ok to not have (not sure if we
>> have scope for such a trade-off)
>>
>> -Nishith
>>
>> On Mon, Dec 23, 2019 at 10:16 AM Sivabalan <n.siv...@gmail.com> wrote:
>>
>> > My 2 cents:
>> > I am also a big fan of code formatting in general, given that its
>> > fully automated. But if that comes at the cost of taking some time off
>> > everyone's time(manual fixing), we need to think through if its really
>> > worth it. Especially, wrt import order, I am not sure if that really
>> adds a
>> > lot of value. Anyone who work with an IDE, all imports are collapsed and
>> no
>> > one gets to see that only. So, what ever order or grouping we follow, it
>> > doesn't matter much. So, having said that, I am not up for spending 10
>> mins
>> > everytime we create or update a PR for this import ordering rule. If I
>> plan
>> > to work on two PRs one followed by other, and if I spend 15 odd mins in
>> > fixing first PR just for code formatting when creating one, I might lose
>> > interest to continue working with 2nd. So, would prefer to avoid spending
>> > time manually for code formatting in general.
>> >
>> >
>> >
>> >
>> >
>> > On Mon, Dec 23, 2019 at 7:43 AM Vinoth Chandar <vin...@apache.org>
>> wrote:
>> >
>> > > Can we exhaustively list all that will be manually even after spotless
>> > > plugin is brought back?
>> > >
>> > > On Mon, Dec 23, 2019 at 3:01 AM leesf <leesf0...@gmail.com> wrote:
>> > >
>> > > > After bringing spotless plugin back to project, it would
>> automatically
>> > > fix
>> > > > comment check error except for import order error, we need to fix
>> this
>> > > > error manually. In Apache Flink/Calcite, we also fix it manually, and
>> > > will
>> > > > also look for other plugins to fix import order error if exist.
>> > > >
>> > > > Best,
>> > > > Leesf
>> > > >
>> > > > Vinoth Chandar <vin...@apache.org> 于2019年12月23日周一 下午4:55写道：
>> > > >
>> > > > > I understand. I am saying - we should automate all of this
>> > formatting..
>> > > > :)
>> > > > >
>> > > > > How do other projects do it? Other folks, who worked on the code
>> > > > > refactoring/formatting, may be you can also chime in?
>> > > > >
>> > > > > On Mon, Dec 23, 2019 at 12:24 AM lamberken <lamber...@163.com>
>> > wrote:
>> > > > >
>> > > > > > Hi @Vinoth,
>> > > > > >
>> > > > > >
>> > > > > > The ImportOrder is a custom rule, IDE may can not reformat codes
>> > > > rightly.
>> > > > > > We can highlight this rule on contributing guide.
>> > > > > >
>> > > > > >
>> > > > > > The new ImportOrder rule split import statements into groups and
>> > > groups
>> > > > > > are separated by one blank line.
>> > > > > > These groups are 1) org.apache.hudi 2) third party imports 3)
>> > > javax
>> > > > > > 4) java 5) static
>> > > > > >
>> > > > > >
>> > > > > > For example
>> > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> /---------------------------------------------------------------------------
>> > > > > > package org.apache.hudi.metrics;
>> > > > > >
>> > > > > > import org.apache.hudi.config.HoodieWriteConfig;
>> > > > > > import org.apache.hudi.exception.HoodieException;
>> > > > > >
>> > > > > > import com.google.common.base.Preconditions;
>> > > > > > import org.apache.log4j.LogManager;
>> > > > > > import org.apache.log4j.Logger;
>> > > > > >
>> > > > > > import javax.management.remote.JMXConnectorServer;
>> > > > > > import javax.management.remote.JMXConnectorServerFactory;
>> > > > > > import javax.management.remote.JMXServiceURL;
>> > > > > >
>> > > > > > import java.io.Closeable;
>> > > > > > import java.lang.management.ManagementFactory;
>> > > > > > import java.rmi.registry.LocateRegistry;
>> > > > > >
>> > > > > > public class JmxMetricsReporter extends MetricsReporter {
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> /---------------------------------------------------------------------------
>> > > > > >
>> > > > > >
>> > > > > > best,
>> > > > > > lamber-ken
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > At 2019-12-23 15:45:27, "Vinoth Chandar" <vin...@apache.org>
>> > wrote:
>> > > > > > >+1 on 1/3 and improving the contributing guide. But on 2, IMO
>> it
>> > > > would
>> > > > > be
>> > > > > > >overloading PULL_REQUEST_TEMPLATE.
>> > > > > > >
>> > > > > > >Bigger point here is: We need a fully automated way of
>> formatting
>> > > code
>> > > > > > >either using IDE or using something like spotless.
>> > > > > > >I pulled the checkstyle rules into intellij and even then I
>> > noticed
>> > > > that
>> > > > > > it
>> > > > > > >does not apply all rules while formatting (e.g line breaks
>> between
>> > > > > import
>> > > > > > >groups).
>> > > > > > >
>> > > > > > >On Sun, Dec 22, 2019 at 11:36 PM lamberken <lamber...@163.com>
>> > > wrote:
>> > > > > > >
>> > > > > > >> Hi Vinoth,
>> > > > > > >>
>> > > > > > >>
>> > > > > > >> Here are some of my points:
>> > > > > > >>
>> > > > > > >>
>> > > > > > >> 1, When developers are not familiar with checkstyle rules,
>> they
>> > > feel
>> > > > > > >> uncomfortable. I think its a good idea to
>> > > > > > >> make the instructions on contributing guide work with the
>> > > checkstyle
>> > > > > > rules
>> > > > > > >> we already have.
>> > > > > > >>
>> > > > > > >>
>> > > > > > >> 2, We can also prompt users in the PULL_REQUEST_TEMPLATE about
>> > how
>> > > > to
>> > > > > > >> check code style by themself manually.
>> > > > > > >>
>> > > > > > >>
>> > > > > > >> 3, The code of the current project has met the checkstyle
>> rules,
>> > > we
>> > > > > just
>> > > > > > >> need handle the incremental codes.
>> > > > > > >>
>> > > > > > >>
>> > > > > > >> 4, here are some useful tools which can be placed on
>> > contributing
>> > > > > guide.
>> > > > > > >> 1) mvn scalastyle:check
>> > > > > > >> 2) mvn checkstyle:check
>> > > > > > >> 3) https://checkstyle.sourceforge.io/index.html
>> > > > > > >> 4) http://www.scalastyle.org/rules-1.0.0.html
>> > > > > > >>
>> > > > > > >>
>> > > > > > >> best,
>> > > > > > >> lamber-ken
>> > > > > > >> On 12/23/2019 13:03，Vinoth Chandar<vin...@apache.org> wrote：
>> > > > > > >> Hello all,
>> > > > > > >>
>> > > > > > >> I know a bunch of work has happened to format the code base,
>> > > closer
>> > > > to
>> > > > > > what
>> > > > > > >> other project are doing..
>> > > > > > >>
>> > > > > > >> While working through some checkstyle violations today, I
>> > noticed
>> > > > that
>> > > > > > the
>> > > > > > >> IDE formatting is now out of date with the checkstyle
>> enforced?
>> > > > > Manually
>> > > > > > >> fixing these checkstyle issues are not a very productive use
>> of
>> > > time
>> > > > > > IMHO.
>> > > > > > >>
>> > > > > > >> http://hudi.apache.org/contributing.html#ide-setup
>> > > > > > >>
>> > > > > > >> Can we make the instructions here work with the checkstyle
>> rules
>> > > we
>> > > > > > already
>> > > > > > >> have? Goal should be that formatting code in IntelliJ (or
>> other
>> > > > IDEs)
>> > > > > > >> should autofix it so that checkstyle passes..
>> > > > > > >>
>> > > > > > >> thoughts?
>> > > > > > >>
>> > > > > > >> Thanks
>> > > > > > >> Vinoth
>> > > > > > >>
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> >
>> > --
>> > Regards,
>> > -Sivabalan
>> >
>>
