You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@dolphinscheduler.apache.org by Chufeng Gao <ch...@gmail.com> on 2022/07/15 06:52:38 UTC
[DISCUSS] Replace Stylecheck plugin with Spotless plugin
Hi developers,
Currently, DS uses `Checkstyle` plugin for style and formatting checks.
However, there are tons of legacy formatting errors in the project popping
out every time we build the project. Thus, in practice, some developers
tend to use `-Dcheckstyle.skip` flag when building the project and this
make the `style check` a little bit awkward.
Huge advantage of using `Spotless`[1][2] over `CheckStyle` is in addition
to check the formatting of the code it also has `apply` goal that fixes all
the style and formatting automatically. With `Spotless`, we could fix the
legacy formatting errors once for all and developers could fix new format
errors with a single line of command `mvn spotless:apply`.
If you have any suggestions, you are very welcome to comment in this mail
thread or in issue #10963 [3]
[1] https://github.com/diffplug/spotless
[2] https://github.com/diffplug/spotless/tree/main/plugin-maven
[3] https://github.com/apache/dolphinscheduler/issues/10963
Thanks!
Best Regards,
Chufeng Gao
Re: [DISCUSS] Replace Stylecheck plugin with Spotless plugin
Posted by Chufeng Gao <ch...@gmail.com>.
Hi developers,
With the help of Zhenxu and Wenjun, we have successfully replaced the
`Checkstyle` with `Spotless` in DolphinScheduler project [1] .
For your convenience, we suggest you copy the `pre-commit hook` file
`/style/pre-commit` to your `.git/hooks/`
directory so that every time you commit your code with `git commit`,
`Spotless` will automatically fix things for you.
In case you do not want to trigger the hook every time you run `git commit`
and choose to fix all commits right before pushing, you may use
`--no-verify` flag when running `git commit`.
If you have any trouble using `Spotless`, please let us know by replying to
this email thread.
Thanks!
[1] https://github.com/apache/dolphinscheduler/pull/11272
*Best Regards,*
*Chufeng (Eric) Gao*
Chufeng Gao <ch...@gmail.com> 于2022年7月15日周五 14:52写道：
> Hi developers,
>
> Currently, DS uses `Checkstyle` plugin for style and formatting checks.
> However, there are tons of legacy formatting errors in the project popping
> out every time we build the project. Thus, in practice, some developers
> tend to use `-Dcheckstyle.skip` flag when building the project and this
> make the `style check` a little bit awkward.
>
> Huge advantage of using `Spotless`[1][2] over `CheckStyle` is in addition
> to check the formatting of the code it also has `apply` goal that fixes all
> the style and formatting automatically. With `Spotless`, we could fix the
> legacy formatting errors once for all and developers could fix new format
> errors with a single line of command `mvn spotless:apply`.
>
> If you have any suggestions, you are very welcome to comment in this mail
> thread or in issue #10963 [3]
>
> [1] https://github.com/diffplug/spotless
> [2] https://github.com/diffplug/spotless/tree/main/plugin-maven
> [3] https://github.com/apache/dolphinscheduler/issues/10963
>
> Thanks!
>
> Best Regards,
>
> Chufeng Gao
>
>
Re: [DISCUSS] Replace Stylecheck plugin with Spotless plugin
Posted by zhuxuetong <59...@qq.com.INVALID>.
+1
> 在 2022年7月15日，14:52，Chufeng Gao <ch...@gmail.com> 写道：
>
> Hi developers,
>
> Currently, DS uses `Checkstyle` plugin for style and formatting checks.
> However, there are tons of legacy formatting errors in the project popping
> out every time we build the project. Thus, in practice, some developers
> tend to use `-Dcheckstyle.skip` flag when building the project and this
> make the `style check` a little bit awkward.
>
> Huge advantage of using `Spotless`[1][2] over `CheckStyle` is in addition
> to check the formatting of the code it also has `apply` goal that fixes all
> the style and formatting automatically. With `Spotless`, we could fix the
> legacy formatting errors once for all and developers could fix new format
> errors with a single line of command `mvn spotless:apply`.
>
> If you have any suggestions, you are very welcome to comment in this mail
> thread or in issue #10963 [3]
>
> [1] https://github.com/diffplug/spotless
> [2] https://github.com/diffplug/spotless/tree/main/plugin-maven
> [3] https://github.com/apache/dolphinscheduler/issues/10963
>
> Thanks!
>
> Best Regards,
>
> Chufeng Gao
>
Re: [DISCUSS] Replace Stylecheck plugin with Spotless plugin
Posted by kezhenxu94 <ke...@apache.org>.
Thanks. This looks good to me.
> On Jul 17, 2022, at 10:47, Chufeng Gao <ch...@gmail.com> wrote:
>
> Hi Zhenxu,
>
> Thanks for the comment. Spotless does support fixing style and formatting
> incrementally. A example here:
>
> <configuration>
> <ratchetFrom>upstream/main</ratchetFrom> <!-- only format files
> which have changed since origin/main -->
> <!-- ... define formats ... -->
> </configuration>
>
> For details, see: `ratchet`
> https://github.com/diffplug/spotless/tree/main/plugin-maven#ratchet
>
> Thanks!
>
> *Best Regards,*
>
> *Chufeng (Eric) Gao*
>
>
>
> kezhenxu94 <ke...@apache.org> 于2022年7月17日周日 10:41写道：
>
>>
>>
>>>> On Jul 15, 2022, at 21:03, Chufeng Gao <ch...@gmail.com> wrote:
>>>
>>> Thanks Wenjun for the comment. I agree that we could fix the code style
>>> incrementally instead of pushing a super big change and blowing up the
>> CI.
>>> Since I'm not familiar with DS CI, may I ask how much work we need to do
>> in
>>> the CI side if we pick up Spotless?
>>
>> If you want to pick up Spotless, once you finished setting up locally,
>> it's just modifying a command in CI (change from checkstyle plugin to
>> spotless plugin)
>> and removing the ReviewDog related commands, should be a small change in
>> CI.
>>
>> My concern is more about whether Spotless can work in/with incremental
>> mode,
>> in another word, developers can `check` AND `apply` ONLY to those files
>> they modified,
>> otherwise it's hard to work if I just change file A and `apply` command
>> fixes all files.
>>
>> Chufeng, can you investigate whether Spotless can work in/with incremental
>> mode and
>> if so, how to do that.
>>
>> Thanks.
Re: [DISCUSS] Replace Stylecheck plugin with Spotless plugin
Posted by Chufeng Gao <ch...@gmail.com>.
Hi Zhenxu,
Thanks for the comment. Spotless does support fixing style and formatting
incrementally. A example here:
<configuration>
<ratchetFrom>upstream/main</ratchetFrom> <!-- only format files
which have changed since origin/main -->
<!-- ... define formats ... -->
</configuration>
For details, see: `ratchet`
https://github.com/diffplug/spotless/tree/main/plugin-maven#ratchet
Thanks!
*Best Regards,*
*Chufeng (Eric) Gao*
kezhenxu94 <ke...@apache.org> 于2022年7月17日周日 10:41写道：
>
>
> > On Jul 15, 2022, at 21:03, Chufeng Gao <ch...@gmail.com> wrote:
> >
> > Thanks Wenjun for the comment. I agree that we could fix the code style
> > incrementally instead of pushing a super big change and blowing up the
> CI.
> > Since I'm not familiar with DS CI, may I ask how much work we need to do
> in
> > the CI side if we pick up Spotless?
>
> If you want to pick up Spotless, once you finished setting up locally,
> it's just modifying a command in CI (change from checkstyle plugin to
> spotless plugin)
> and removing the ReviewDog related commands, should be a small change in
> CI.
>
> My concern is more about whether Spotless can work in/with incremental
> mode,
> in another word, developers can `check` AND `apply` ONLY to those files
> they modified,
> otherwise it's hard to work if I just change file A and `apply` command
> fixes all files.
>
> Chufeng, can you investigate whether Spotless can work in/with incremental
> mode and
> if so, how to do that.
>
> Thanks.
Re: [DISCUSS] Replace Stylecheck plugin with Spotless plugin
Posted by kezhenxu94 <ke...@apache.org>.
> On Jul 15, 2022, at 21:03, Chufeng Gao <ch...@gmail.com> wrote:
>
> Thanks Wenjun for the comment. I agree that we could fix the code style
> incrementally instead of pushing a super big change and blowing up the CI.
> Since I'm not familiar with DS CI, may I ask how much work we need to do in
> the CI side if we pick up Spotless?
If you want to pick up Spotless, once you finished setting up locally,
it's just modifying a command in CI (change from checkstyle plugin to spotless plugin)
and removing the ReviewDog related commands, should be a small change in CI.
My concern is more about whether Spotless can work in/with incremental mode,
in another word, developers can `check` AND `apply` ONLY to those files they modified,
otherwise it's hard to work if I just change file A and `apply` command fixes all files.
Chufeng, can you investigate whether Spotless can work in/with incremental mode and
if so, how to do that.
Thanks.
Re: [DISCUSS] Replace Stylecheck plugin with Spotless plugin
Posted by Chufeng Gao <ch...@gmail.com>.
Thanks Wenjun for the comment. I agree that we could fix the code style
incrementally instead of pushing a super big change and blowing up the CI.
Since I'm not familiar with DS CI, may I ask how much work we need to do in
the CI side if we pick up Spotless?
Thanks.
*Best Regards,*
*Chufeng (Eric) Gao*
Wenjun Ruan <be...@gmail.com> 于2022年7月15日周五 16:03写道：
> +1.
>
> Since we have closed the checkstyle in CI in a long time, right now
> there is a lot of files breaking our checkstyle rule, but we have
> opened the checkstyle again, the code style will be fixed later.
>
> The problem with using checkstyle is that our checkstyle doesn't cover
> all code style, e.g. chained method call, method arguments alian... I
> am not sure if it can be improved by adding new checkstyle rules.
> In my knowledge, spotless can help to format the code in one command
> and doesn't need to rely on ide setting, this can help to make our
> code style really consistent.
>
> But I think if we use spotless, we still need to increment fix the
> code style, rather than fix it all in one PR.
>
> Thanks,
> Wenjun
>
> On Fri, Jul 15, 2022 at 2:53 PM Chufeng Gao <ch...@gmail.com> wrote:
> >
> > Hi developers,
> >
> > Currently, DS uses `Checkstyle` plugin for style and formatting checks.
> > However, there are tons of legacy formatting errors in the project
> popping
> > out every time we build the project. Thus, in practice, some developers
> > tend to use `-Dcheckstyle.skip` flag when building the project and this
> > make the `style check` a little bit awkward.
> >
> > Huge advantage of using `Spotless`[1][2] over `CheckStyle` is in addition
> > to check the formatting of the code it also has `apply` goal that fixes
> all
> > the style and formatting automatically. With `Spotless`, we could fix the
> > legacy formatting errors once for all and developers could fix new format
> > errors with a single line of command `mvn spotless:apply`.
> >
> > If you have any suggestions, you are very welcome to comment in this mail
> > thread or in issue #10963 [3]
> >
> > [1] https://github.com/diffplug/spotless
> > [2] https://github.com/diffplug/spotless/tree/main/plugin-maven
> > [3] https://github.com/apache/dolphinscheduler/issues/10963
> >
> > Thanks!
> >
> > Best Regards,
> >
> > Chufeng Gao
>
Re: [DISCUSS] Replace Stylecheck plugin with Spotless plugin
Posted by Wenjun Ruan <be...@gmail.com>.
+1.
Since we have closed the checkstyle in CI in a long time, right now
there is a lot of files breaking our checkstyle rule, but we have
opened the checkstyle again, the code style will be fixed later.
The problem with using checkstyle is that our checkstyle doesn't cover
all code style, e.g. chained method call, method arguments alian... I
am not sure if it can be improved by adding new checkstyle rules.
In my knowledge, spotless can help to format the code in one command
and doesn't need to rely on ide setting, this can help to make our
code style really consistent.
But I think if we use spotless, we still need to increment fix the
code style, rather than fix it all in one PR.
Thanks,
Wenjun
On Fri, Jul 15, 2022 at 2:53 PM Chufeng Gao <ch...@gmail.com> wrote:
>
> Hi developers,
>
> Currently, DS uses `Checkstyle` plugin for style and formatting checks.
> However, there are tons of legacy formatting errors in the project popping
> out every time we build the project. Thus, in practice, some developers
> tend to use `-Dcheckstyle.skip` flag when building the project and this
> make the `style check` a little bit awkward.
>
> Huge advantage of using `Spotless`[1][2] over `CheckStyle` is in addition
> to check the formatting of the code it also has `apply` goal that fixes all
> the style and formatting automatically. With `Spotless`, we could fix the
> legacy formatting errors once for all and developers could fix new format
> errors with a single line of command `mvn spotless:apply`.
>
> If you have any suggestions, you are very welcome to comment in this mail
> thread or in issue #10963 [3]
>
> [1] https://github.com/diffplug/spotless
> [2] https://github.com/diffplug/spotless/tree/main/plugin-maven
> [3] https://github.com/apache/dolphinscheduler/issues/10963
>
> Thanks!
>
> Best Regards,
>
> Chufeng Gao
Re: [DISCUSS] Replace Stylecheck plugin with Spotless plugin
Posted by zhuxuetong <59...@qq.com.INVALID>.
Another solution is to use Spotless to modify the format of all the code, and then fix CheckStyle.xml, still using CheckStyle in DolphinScheduler. This does not require modifying the CI logic.
> 在 2022年7月15日，14:52，Chufeng Gao <ch...@gmail.com> 写道：
>
> Hi developers,
>
> Currently, DS uses `Checkstyle` plugin for style and formatting checks.
> However, there are tons of legacy formatting errors in the project popping
> out every time we build the project. Thus, in practice, some developers
> tend to use `-Dcheckstyle.skip` flag when building the project and this
> make the `style check` a little bit awkward.
>
> Huge advantage of using `Spotless`[1][2] over `CheckStyle` is in addition
> to check the formatting of the code it also has `apply` goal that fixes all
> the style and formatting automatically. With `Spotless`, we could fix the
> legacy formatting errors once for all and developers could fix new format
> errors with a single line of command `mvn spotless:apply`.
>
> If you have any suggestions, you are very welcome to comment in this mail
> thread or in issue #10963 [3]
>
> [1] https://github.com/diffplug/spotless
> [2] https://github.com/diffplug/spotless/tree/main/plugin-maven
> [3] https://github.com/apache/dolphinscheduler/issues/10963
>
> Thanks!
>
> Best Regards,
>
> Chufeng Gao
>