You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@doris.apache.org by morrysnow <mo...@126.com> on 2022/05/06 07:02:53 UTC
Re: Add more rules to checkstyle.xml in fe
Hi devs,
I create issues for every steps. Any one willing to submit pr could assign issue to himself/herself.
Step1: https://github.com/apache/incubator-doris/issues/9403 <https://github.com/apache/incubator-doris/issues/9403>
Step2: https://github.com/apache/incubator-doris/issues/9404
Step3: https://github.com/apache/incubator-doris/issues/9405
Step4: https://github.com/apache/incubator-doris/issues/9406
> 2022年4月29日 13:43，morrysnow <mo...@126.com> 写道：
>
> Hi, devs
>
> I want to add Step 0 before all steps.
>
> Step 0:
>
> Add checkstyle action to github action to check pr style. This action checks incremental code and adds comment to pr automatically.
> Action: https://github.com/dbelyaev/action-checkstyle <https://github.com/dbelyaev/action-checkstyle>
> Demo: https://github.com/morrySnow/test_checkstyle/pull/5 <https://github.com/morrySnow/test_checkstyle/pull/5>
>
>> 2022年4月29日 13:40，morrysnow <mo...@126.com> 写道：
>>
>> Hi, mingyu
>>
>> 1. Check license is needed for local check.
>> 2. Add a rule to forbidden static import is ok for me
>>
>>> 2022年4月28日 21:34，陈明雨 <mo...@163.com> 写道：
>>>
>>> That would be great!
>>> Some suggestion about the rules:
>>>
>>>
>>>> d. license header check
>>> For now, we use skywalking-eyes to check license header, is this rule still needed?
>>>
>>>
>>>> adjust the order of import to "static, org.apache.doris, third party, java"
>>> Better not using static import
>>>
>>>
>>>
>>>
>>> --
>>>
>>> 此致！Best Regards
>>> 陈明雨 Mingyu Chen
>>>
>>> Email:
>>> chenmingyu@apache.org
>>>
>>>
>>>
>>>
>>>
>>> 在 2022-04-28 17:14:34，"morrysnow" <mo...@126.com> 写道：
>>>> Hi, devs,
>>>>
>>>> I split adding java check style into 5 steps. I’m willing to get feedback about it.
>>>>
>>>> CheckStyle and Spotless will modify about 10k lines.
>>>>
>>>> Step 1:
>>>>
>>>> 1. Add Spotless plugin and do some simple format(1198 files changed, 3205 insertions(+), 4350 deletions(-))
>>>> a. end with new line
>>>> b. trim trailing whitespace
>>>> c. replace tab indent with 4 spaces indent
>>>> d. license header check
>>>> e. adjust the order of import to "static, org.apache.doris, third party, java"
>>>> f. remove unused imports
>>>> g. make sure all files are encoding by utf-8
>>>> h. make sure all files line ending by LF
>>>>
>>>> 2. Add checkstyle rules but set default severity to warning to avoid compile failed.
>>>>
>>>> 3. set some rules' severity to error in Checkstyle since no lines in doris break these rules(0 files, 0 lines)
>>>> a. Merge conflicts unresolved
>>>> b. Avoid using corresponding octal or Unicode escape
>>>> c. Avoid Escaped Unicode Characters
>>>> d. No Line Wrap
>>>> e. Package Name
>>>> f. Type Name
>>>> g. Annotation Location
>>>> h. Interface Type Parameter
>>>> i. CatchParameterName
>>>> j. Pattern Variable Name
>>>> k. Record Component Name
>>>> l. Record Type Parameter Name
>>>> m. Method Type Parameter Name
>>>>
>>>> 4. Set below rules severity to error in Checkstyle since these had done by splotless(12 files, 177 lines)
>>>> a. Redundant Import
>>>> b. Custom Import Order
>>>> c. Unused Imports
>>>> d. Avoid Star Import
>>>> e. tab character in file
>>>> f. Newline At End Of File
>>>> g. Trailing whitespace found
>>>>
>>>>
>>>>
>>>> Step 2: (241 files, 962 lines (with Step 1))
>>>>
>>>> 1. Set below rules' severity to error in Checkstyle, all rules are import to make sure the code is correct
>>>> a. Need Braces
>>>> b. Equals Hash Code
>>>> c. Missing Switch Default
>>>> d. Fall Through
>>>> e. No Finalizer
>>>>
>>>>
>>>> 2. Set below rules' severity to error in Checkstyle, all rules are about name
>>>> a. Member Name
>>>> b. Constant Name
>>>> c. Parameter Name
>>>> d. LambdaParameterName
>>>> e. Local Variable Name
>>>> f. Class Type Parameter Name
>>>> g. Static Variable Name
>>>> h. Method Name
>>>> j. Abbreviation As Word In Name
>>>>
>>>>
>>>> Step 3: (605 files, 6008 lines (with Step 1 and Step 2))
>>>>
>>>> 1. Set below rules' severity to error in Checkstyle, all rules are about whitespace
>>>> a. Empty Block
>>>> b. Left Curly
>>>> c. Right Curly
>>>> d. White space Around
>>>> e. Generic Whitespace
>>>> f. Indentation
>>>> g. No Whitespace Before Case Default Colon
>>>> h. Method Param Pad
>>>> i. Whitespace
>>>> j. Paren Pad
>>>> k. Extra newline
>>>> l. Extra newline at end of block
>>>> m. Empty Statement
>>>> n. Empty Catch Block
>>>> o. Comments Indentation
>>>>
>>>> 2. Set below rules' severity to error in Checkstyle, all rules are about the position of the newline
>>>> a. Line Length 120
>>>> b. Operator Wrap
>>>> c. One Statement Per Line
>>>> d. Multiple Variable Declarations
>>>>
>>>>
>>>> Step 4: (674 files, 6730 lines (with Step 1, Step 2 and Step 3))
>>>>
>>>> 1. Set below rules' severity to error in Checkstyle
>>>> a. Array Type Style
>>>> b. Upper Ell
>>>> c. Modifier Order
>>>> d. Empty Line Separator
>>>> e. Separator Wrap
>>>> f. Overload Methods Declaration Order
>>>> g. Variable Declaration Usage Distance
>>>> i. One Top Level Class
>>>>
>>>>
>>>> Step 5:
>>>>
>>>> 1. Add java doc check to CheckStyle and set severity to warning.
>>>>
>>>> 2. Fix java doc warning gradually
>>>>
>>>>
>>>>> 2022年4月28日 00:39，morrysnow <mo...@126.com> 写道：
>>>>>
>>>>> Hi, devs
>>>>>
>>>>> I summarized all the rules I wanted to add. I examined the number of lines of code affected by all the rules and categorized them by importance.
>>>>>
>>>>> The link is here: https://github.com/apache/incubator-doris/issues/8985#issuecomment-1111214162 <https://github.com/apache/incubator-doris/issues/8985#issuecomment-1111214162>
>>>>>
>>>>> I would like to be able to submit them in multiple patches. The principle of patch splitting is as follows. Those with low impact and high importance are submitted first and sets the severity to Error. The Javadoc commits last and sets the severity to Warning.
>>>>>
>>>>>
>>>>>> 2022年4月18日 16:09，vin jake <ja...@gmail.com> 写道：
>>>>>>
>>>>>> Hi, there is a new PR about format doris.
>>>>>> https://github.com/apache/incubator-doris/pull/9072
>>>>>>
>>>>>> I think it's time to ensure what checkstyle rules we should add for fe.
>>>>>>
>>>>>> All people who care about format rule can put up your thoughts.
>>>>>>
>>>>>> We can add it in the issue:
>>>>>> https://github.com/apache/incubator-doris/issues/8985
>>>>>>
>>>>>>
>>>>>> On Thu, Apr 14, 2022 at 6:52 PM vin jake <ja...@gmail.com> wrote:
>>>>>>
>>>>>>> great, I will trim it in my PR.
>>>>>>>
>>>>>>> checkstyle.xml need more discussion
>>>>>>>
>>>>>>> On Thu, Apr 14, 2022 at 5:48 PM morrysnow <mo...@126.com> wrote:
>>>>>>>
>>>>>>>> I agree with u.
>>>>>>>>
>>>>>>>> For the first point, I want to list rules first, and then change
>>>>>>>> checkstyle.xml.
>>>>>>>>
>>>>>>>> For the second point, original change information in git is not lost, we
>>>>>>>> just need to do blame on the version prior to the ‘code style’ commit and
>>>>>>>> some gui tools could list history of one file. Anyway, it is not very
>>>>>>>> convenience.
>>>>>>>>
>>>>>>>>> 2022年4月14日 17:35，Shuo Wang <wa...@gmail.com> 写道：
>>>>>>>>>
>>>>>>>>> In general, I believe that we should do some work to make the code clean
>>>>>>>>> and readable.
>>>>>>>>>
>>>>>>>>> My concern is:
>>>>>>>>> 1. We should have an agreement on the code style specification in the
>>>>>>>>> community at first.
>>>>>>>>> 2. If the many lines of code change after applying the code style rule,
>>>>>>>> we
>>>>>>>>> would lose the original changelog via `git blame`.
>>>>>>>>>
>>>>>>>>> vin jake <ja...@gmail.com> 于2022年4月14日周四 17:12写道：
>>>>>>>>>
>>>>>>>>>> I have add it in PR
>>>>>>>> https://github.com/apache/incubator-doris/pull/8987
>>>>>>>>>>
>>>>>>>>>> On Thu, Apr 14, 2022 at 4:37 PM morrysnow <mo...@126.com> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi, devs,
>>>>>>>>>>>
>>>>>>>>>>> Currently, we only have two rules in checkstyle.xml in fe. These are
>>>>>>>> all
>>>>>>>>>>> about import. So, the code style in fe is very casual.
>>>>>>>>>>> I want to add more rules to checkstyle.xml in fe to Improve code
>>>>>>>>>>> readability, and adjust all fe code to satisfy new code style step by
>>>>>>>>>> step.
>>>>>>>>>>> What do you think about it? If this is a good idea. I will research
>>>>>>>> which
>>>>>>>>>>> rules apply to our code and put together a list.
>>>>>>>>>>>
>>>>>>>>>>> ---------------------------------------------------------------------
>>>>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
>>>>>>>>>>> For additional commands, e-mail: dev-help@doris.apache.org
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> ---------------------------------------------------------------------
>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
>>>>>>>> For additional commands, e-mail: dev-help@doris.apache.org
>>>>>>>>
>>>>>>>>
>>>>>
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
>>>> For additional commands, e-mail: dev-help@doris.apache.org
>
Re:Re:Re: Add more rules to checkstyle.xml in fe
Posted by 陈明雨 <mo...@163.com>.
The FE java format check is now required.
If you need to check it in local env, please refer to http://doris.incubator.apache.org/developer/developer-guide/java-format-code.html#import-order
--
此致！Best Regards
陈明雨 Mingyu Chen
Email:
morningman@apache.org
在 2022-06-14 22:00:09，"陈明雨" <mo...@163.com> 写道：
>I'm not sure if we currently have some overly restrictive rules that the developer hard to follow.
>
>However, I think we can try to turn on the required check, observe the recently submitted PR, and if any unreasonable checks are found,
>modify them in time to prevent such checks from affecting the efficiency of code merging.
>
>
>
>
>--
>
>此致！Best Regards
>陈明雨 Mingyu Chen
>
>Email:
>morningman@apache.org
>
>
>
>
>
>At 2022-06-14 21:36:26, "morrysnow" <mo...@126.com> wrote:
>>Hi , all devs
>>
>>After this PR: https://github.com/apache/incubator-doris/pull/10134 <https://github.com/apache/incubator-doris/pull/10134>, all checkstyle error has been fixed.
>>So, i want to turn Github workflow 'FE Code Style Checker' to 'Required' after this PR be merged.
>>
>>After #10134 be merged, when compiling fe with `maven`, `CheckStyle` checks are done by default.
>>This will slightly slow down compilation. If you want to skip checkstyle, please use the following command to compile
>>
>>mvn clean install -DskipTests -Dcheckstyle.skip
>>
>>Also, you can check your code style seperately by run following command under fe directory:
>>
>>mvn checkstyle:check
>>
>>
>>> 2022年5月6日 16:16，ling miao <li...@apache.org> 写道：
>>>
>>> Regarding the code format correction of the old code, do we currently need
>>> to do it manually?
>>>
>>> Ling Miao
>>>
>>> morrysnow <mo...@126.com> 于2022年5月6日周五 15:03写道：
>>>
>>>> Hi devs,
>>>>
>>>> I create issues for every steps. Any one willing to submit pr could assign
>>>> issue to himself/herself.
>>>>
>>>> Step1: https://github.com/apache/incubator-doris/issues/9403 <
>>>> https://github.com/apache/incubator-doris/issues/9403>
>>>> Step2: https://github.com/apache/incubator-doris/issues/9404
>>>> Step3: https://github.com/apache/incubator-doris/issues/9405
>>>> Step4: https://github.com/apache/incubator-doris/issues/9406
>>>>
>>>>> 2022年4月29日 13:43，morrysnow <mo...@126.com> 写道：
>>>>>
>>>>> Hi, devs
>>>>>
>>>>> I want to add Step 0 before all steps.
>>>>>
>>>>> Step 0:
>>>>>
>>>>> Add checkstyle action to github action to check pr style. This action
>>>> checks incremental code and adds comment to pr automatically.
>>>>> Action: https://github.com/dbelyaev/action-checkstyle <
>>>> https://github.com/dbelyaev/action-checkstyle>
>>>>> Demo: https://github.com/morrySnow/test_checkstyle/pull/5 <
>>>> https://github.com/morrySnow/test_checkstyle/pull/5>
>>>>>
>>>>>> 2022年4月29日 13:40，morrysnow <mo...@126.com> 写道：
>>>>>>
>>>>>> Hi, mingyu
>>>>>>
>>>>>> 1. Check license is needed for local check.
>>>>>> 2. Add a rule to forbidden static import is ok for me
>>>>>>
>>>>>>> 2022年4月28日 21:34，陈明雨 <mo...@163.com> 写道：
>>>>>>>
>>>>>>> That would be great!
>>>>>>> Some suggestion about the rules:
>>>>>>>
>>>>>>>
>>>>>>>> d. license header check
>>>>>>> For now, we use skywalking-eyes to check license header, is this rule
>>>> still needed?
>>>>>>>
>>>>>>>
>>>>>>>> adjust the order of import to "static, org.apache.doris, third party,
>>>> java"
>>>>>>> Better not using static import
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> 此致！Best Regards
>>>>>>> 陈明雨 Mingyu Chen
>>>>>>>
>>>>>>> Email:
>>>>>>> chenmingyu@apache.org
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 在 2022-04-28 17:14:34，"morrysnow" <mo...@126.com> 写道：
>>>>>>>> Hi, devs,
>>>>>>>>
>>>>>>>> I split adding java check style into 5 steps. I’m willing to get
>>>> feedback about it.
>>>>>>>>
>>>>>>>> CheckStyle and Spotless will modify about 10k lines.
>>>>>>>>
>>>>>>>> Step 1:
>>>>>>>>
>>>>>>>> 1. Add Spotless plugin and do some simple format(1198 files changed,
>>>> 3205 insertions(+), 4350 deletions(-))
>>>>>>>> a. end with new line
>>>>>>>> b. trim trailing whitespace
>>>>>>>> c. replace tab indent with 4 spaces indent
>>>>>>>> d. license header check
>>>>>>>> e. adjust the order of import to "static, org.apache.doris, third
>>>> party, java"
>>>>>>>> f. remove unused imports
>>>>>>>> g. make sure all files are encoding by utf-8
>>>>>>>> h. make sure all files line ending by LF
>>>>>>>>
>>>>>>>> 2. Add checkstyle rules but set default severity to warning to avoid
>>>> compile failed.
>>>>>>>>
>>>>>>>> 3. set some rules' severity to error in Checkstyle since no lines in
>>>> doris break these rules(0 files, 0 lines)
>>>>>>>> a. Merge conflicts unresolved
>>>>>>>> b. Avoid using corresponding octal or Unicode escape
>>>>>>>> c. Avoid Escaped Unicode Characters
>>>>>>>> d. No Line Wrap
>>>>>>>> e. Package Name
>>>>>>>> f. Type Name
>>>>>>>> g. Annotation Location
>>>>>>>> h. Interface Type Parameter
>>>>>>>> i. CatchParameterName
>>>>>>>> j. Pattern Variable Name
>>>>>>>> k. Record Component Name
>>>>>>>> l. Record Type Parameter Name
>>>>>>>> m. Method Type Parameter Name
>>>>>>>>
>>>>>>>> 4. Set below rules severity to error in Checkstyle since these had
>>>> done by splotless(12 files, 177 lines)
>>>>>>>> a. Redundant Import
>>>>>>>> b. Custom Import Order
>>>>>>>> c. Unused Imports
>>>>>>>> d. Avoid Star Import
>>>>>>>> e. tab character in file
>>>>>>>> f. Newline At End Of File
>>>>>>>> g. Trailing whitespace found
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Step 2: (241 files, 962 lines (with Step 1))
>>>>>>>>
>>>>>>>> 1. Set below rules' severity to error in Checkstyle, all rules are
>>>> import to make sure the code is correct
>>>>>>>> a. Need Braces
>>>>>>>> b. Equals Hash Code
>>>>>>>> c. Missing Switch Default
>>>>>>>> d. Fall Through
>>>>>>>> e. No Finalizer
>>>>>>>>
>>>>>>>>
>>>>>>>> 2. Set below rules' severity to error in Checkstyle, all rules are
>>>> about name
>>>>>>>> a. Member Name
>>>>>>>> b. Constant Name
>>>>>>>> c. Parameter Name
>>>>>>>> d. LambdaParameterName
>>>>>>>> e. Local Variable Name
>>>>>>>> f. Class Type Parameter Name
>>>>>>>> g. Static Variable Name
>>>>>>>> h. Method Name
>>>>>>>> j. Abbreviation As Word In Name
>>>>>>>>
>>>>>>>>
>>>>>>>> Step 3: (605 files, 6008 lines (with Step 1 and Step 2))
>>>>>>>>
>>>>>>>> 1. Set below rules' severity to error in Checkstyle, all rules are
>>>> about whitespace
>>>>>>>> a. Empty Block
>>>>>>>> b. Left Curly
>>>>>>>> c. Right Curly
>>>>>>>> d. White space Around
>>>>>>>> e. Generic Whitespace
>>>>>>>> f. Indentation
>>>>>>>> g. No Whitespace Before Case Default Colon
>>>>>>>> h. Method Param Pad
>>>>>>>> i. Whitespace
>>>>>>>> j. Paren Pad
>>>>>>>> k. Extra newline
>>>>>>>> l. Extra newline at end of block
>>>>>>>> m. Empty Statement
>>>>>>>> n. Empty Catch Block
>>>>>>>> o. Comments Indentation
>>>>>>>>
>>>>>>>> 2. Set below rules' severity to error in Checkstyle, all rules are
>>>> about the position of the newline
>>>>>>>> a. Line Length 120
>>>>>>>> b. Operator Wrap
>>>>>>>> c. One Statement Per Line
>>>>>>>> d. Multiple Variable Declarations
>>>>>>>>
>>>>>>>>
>>>>>>>> Step 4: (674 files, 6730 lines (with Step 1, Step 2 and Step 3))
>>>>>>>>
>>>>>>>> 1. Set below rules' severity to error in Checkstyle
>>>>>>>> a. Array Type Style
>>>>>>>> b. Upper Ell
>>>>>>>> c. Modifier Order
>>>>>>>> d. Empty Line Separator
>>>>>>>> e. Separator Wrap
>>>>>>>> f. Overload Methods Declaration Order
>>>>>>>> g. Variable Declaration Usage Distance
>>>>>>>> i. One Top Level Class
>>>>>>>>
>>>>>>>>
>>>>>>>> Step 5:
>>>>>>>>
>>>>>>>> 1. Add java doc check to CheckStyle and set severity to warning.
>>>>>>>>
>>>>>>>> 2. Fix java doc warning gradually
>>>>>>>>
>>>>>>>>
>>>>>>>>> 2022年4月28日 00:39，morrysnow <mo...@126.com> 写道：
>>>>>>>>>
>>>>>>>>> Hi, devs
>>>>>>>>>
>>>>>>>>> I summarized all the rules I wanted to add. I examined the number of
>>>> lines of code affected by all the rules and categorized them by importance.
>>>>>>>>>
>>>>>>>>> The link is here:
>>>> https://github.com/apache/incubator-doris/issues/8985#issuecomment-1111214162
>>>> <
>>>> https://github.com/apache/incubator-doris/issues/8985#issuecomment-1111214162
>>>>>
>>>>>>>>>
>>>>>>>>> I would like to be able to submit them in multiple patches. The
>>>> principle of patch splitting is as follows. Those with low impact and high
>>>> importance are submitted first and sets the severity to Error. The Javadoc
>>>> commits last and sets the severity to Warning.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> 2022年4月18日 16:09，vin jake <ja...@gmail.com> 写道：
>>>>>>>>>>
>>>>>>>>>> Hi, there is a new PR about format doris.
>>>>>>>>>> https://github.com/apache/incubator-doris/pull/9072
>>>>>>>>>>
>>>>>>>>>> I think it's time to ensure what checkstyle rules we should add
>>>> for fe.
>>>>>>>>>>
>>>>>>>>>> All people who care about format rule can put up your thoughts.
>>>>>>>>>>
>>>>>>>>>> We can add it in the issue:
>>>>>>>>>> https://github.com/apache/incubator-doris/issues/8985
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Thu, Apr 14, 2022 at 6:52 PM vin jake <ja...@gmail.com>
>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> great, I will trim it in my PR.
>>>>>>>>>>>
>>>>>>>>>>> checkstyle.xml need more discussion
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Apr 14, 2022 at 5:48 PM morrysnow <mo...@126.com>
>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> I agree with u.
>>>>>>>>>>>>
>>>>>>>>>>>> For the first point, I want to list rules first, and then change
>>>>>>>>>>>> checkstyle.xml.
>>>>>>>>>>>>
>>>>>>>>>>>> For the second point, original change information in git is not
>>>> lost, we
>>>>>>>>>>>> just need to do blame on the version prior to the ‘code style’
>>>> commit and
>>>>>>>>>>>> some gui tools could list history of one file. Anyway, it is not
>>>> very
>>>>>>>>>>>> convenience.
>>>>>>>>>>>>
>>>>>>>>>>>>> 2022年4月14日 17:35，Shuo Wang <wa...@gmail.com> 写道：
>>>>>>>>>>>>>
>>>>>>>>>>>>> In general, I believe that we should do some work to make the
>>>> code clean
>>>>>>>>>>>>> and readable.
>>>>>>>>>>>>>
>>>>>>>>>>>>> My concern is:
>>>>>>>>>>>>> 1. We should have an agreement on the code style specification
>>>> in the
>>>>>>>>>>>>> community at first.
>>>>>>>>>>>>> 2. If the many lines of code change after applying the code
>>>> style rule,
>>>>>>>>>>>> we
>>>>>>>>>>>>> would lose the original changelog via `git blame`.
>>>>>>>>>>>>>
>>>>>>>>>>>>> vin jake <ja...@gmail.com> 于2022年4月14日周四 17:12写道：
>>>>>>>>>>>>>
>>>>>>>>>>>>>> I have add it in PR
>>>>>>>>>>>> https://github.com/apache/incubator-doris/pull/8987
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Thu, Apr 14, 2022 at 4:37 PM morrysnow <mo...@126.com>
>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi, devs,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Currently, we only have two rules in checkstyle.xml in fe.
>>>> These are
>>>>>>>>>>>> all
>>>>>>>>>>>>>>> about import. So, the code style in fe is very casual.
>>>>>>>>>>>>>>> I want to add more rules to checkstyle.xml in fe to Improve
>>>> code
>>>>>>>>>>>>>>> readability, and adjust all fe code to satisfy new code style
>>>> step by
>>>>>>>>>>>>>> step.
>>>>>>>>>>>>>>> What do you think about it? If this is a good idea. I will
>>>> research
>>>>>>>>>>>> which
>>>>>>>>>>>>>>> rules apply to our code and put together a list.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>> ---------------------------------------------------------------------
>>>>>>>>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
>>>>>>>>>>>>>>> For additional commands, e-mail: dev-help@doris.apache.org
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>> ---------------------------------------------------------------------
>>>>>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
>>>>>>>>>>>> For additional commands, e-mail: dev-help@doris.apache.org
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> ---------------------------------------------------------------------
>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
>>>>>>>> For additional commands, e-mail: dev-help@doris.apache.org
>>>>>
>>>>
>>>>
>>>
>>> --
>>> Ling Miao | Apache Doris
>>
Re:Re: Add more rules to checkstyle.xml in fe
Posted by 陈明雨 <mo...@163.com>.
I'm not sure if we currently have some overly restrictive rules that the developer hard to follow.
However, I think we can try to turn on the required check, observe the recently submitted PR, and if any unreasonable checks are found,
modify them in time to prevent such checks from affecting the efficiency of code merging.
--
此致！Best Regards
陈明雨 Mingyu Chen
Email:
morningman@apache.org
At 2022-06-14 21:36:26, "morrysnow" <mo...@126.com> wrote:
>Hi , all devs
>
>After this PR: https://github.com/apache/incubator-doris/pull/10134 <https://github.com/apache/incubator-doris/pull/10134>, all checkstyle error has been fixed.
>So, i want to turn Github workflow 'FE Code Style Checker' to 'Required' after this PR be merged.
>
>After #10134 be merged, when compiling fe with `maven`, `CheckStyle` checks are done by default.
>This will slightly slow down compilation. If you want to skip checkstyle, please use the following command to compile
>
>mvn clean install -DskipTests -Dcheckstyle.skip
>
>Also, you can check your code style seperately by run following command under fe directory:
>
>mvn checkstyle:check
>
>
>> 2022年5月6日 16:16，ling miao <li...@apache.org> 写道：
>>
>> Regarding the code format correction of the old code, do we currently need
>> to do it manually?
>>
>> Ling Miao
>>
>> morrysnow <mo...@126.com> 于2022年5月6日周五 15:03写道：
>>
>>> Hi devs,
>>>
>>> I create issues for every steps. Any one willing to submit pr could assign
>>> issue to himself/herself.
>>>
>>> Step1: https://github.com/apache/incubator-doris/issues/9403 <
>>> https://github.com/apache/incubator-doris/issues/9403>
>>> Step2: https://github.com/apache/incubator-doris/issues/9404
>>> Step3: https://github.com/apache/incubator-doris/issues/9405
>>> Step4: https://github.com/apache/incubator-doris/issues/9406
>>>
>>>> 2022年4月29日 13:43，morrysnow <mo...@126.com> 写道：
>>>>
>>>> Hi, devs
>>>>
>>>> I want to add Step 0 before all steps.
>>>>
>>>> Step 0:
>>>>
>>>> Add checkstyle action to github action to check pr style. This action
>>> checks incremental code and adds comment to pr automatically.
>>>> Action: https://github.com/dbelyaev/action-checkstyle <
>>> https://github.com/dbelyaev/action-checkstyle>
>>>> Demo: https://github.com/morrySnow/test_checkstyle/pull/5 <
>>> https://github.com/morrySnow/test_checkstyle/pull/5>
>>>>
>>>>> 2022年4月29日 13:40，morrysnow <mo...@126.com> 写道：
>>>>>
>>>>> Hi, mingyu
>>>>>
>>>>> 1. Check license is needed for local check.
>>>>> 2. Add a rule to forbidden static import is ok for me
>>>>>
>>>>>> 2022年4月28日 21:34，陈明雨 <mo...@163.com> 写道：
>>>>>>
>>>>>> That would be great!
>>>>>> Some suggestion about the rules:
>>>>>>
>>>>>>
>>>>>>> d. license header check
>>>>>> For now, we use skywalking-eyes to check license header, is this rule
>>> still needed?
>>>>>>
>>>>>>
>>>>>>> adjust the order of import to "static, org.apache.doris, third party,
>>> java"
>>>>>> Better not using static import
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>>
>>>>>> 此致！Best Regards
>>>>>> 陈明雨 Mingyu Chen
>>>>>>
>>>>>> Email:
>>>>>> chenmingyu@apache.org
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> 在 2022-04-28 17:14:34，"morrysnow" <mo...@126.com> 写道：
>>>>>>> Hi, devs,
>>>>>>>
>>>>>>> I split adding java check style into 5 steps. I’m willing to get
>>> feedback about it.
>>>>>>>
>>>>>>> CheckStyle and Spotless will modify about 10k lines.
>>>>>>>
>>>>>>> Step 1:
>>>>>>>
>>>>>>> 1. Add Spotless plugin and do some simple format(1198 files changed,
>>> 3205 insertions(+), 4350 deletions(-))
>>>>>>> a. end with new line
>>>>>>> b. trim trailing whitespace
>>>>>>> c. replace tab indent with 4 spaces indent
>>>>>>> d. license header check
>>>>>>> e. adjust the order of import to "static, org.apache.doris, third
>>> party, java"
>>>>>>> f. remove unused imports
>>>>>>> g. make sure all files are encoding by utf-8
>>>>>>> h. make sure all files line ending by LF
>>>>>>>
>>>>>>> 2. Add checkstyle rules but set default severity to warning to avoid
>>> compile failed.
>>>>>>>
>>>>>>> 3. set some rules' severity to error in Checkstyle since no lines in
>>> doris break these rules(0 files, 0 lines)
>>>>>>> a. Merge conflicts unresolved
>>>>>>> b. Avoid using corresponding octal or Unicode escape
>>>>>>> c. Avoid Escaped Unicode Characters
>>>>>>> d. No Line Wrap
>>>>>>> e. Package Name
>>>>>>> f. Type Name
>>>>>>> g. Annotation Location
>>>>>>> h. Interface Type Parameter
>>>>>>> i. CatchParameterName
>>>>>>> j. Pattern Variable Name
>>>>>>> k. Record Component Name
>>>>>>> l. Record Type Parameter Name
>>>>>>> m. Method Type Parameter Name
>>>>>>>
>>>>>>> 4. Set below rules severity to error in Checkstyle since these had
>>> done by splotless(12 files, 177 lines)
>>>>>>> a. Redundant Import
>>>>>>> b. Custom Import Order
>>>>>>> c. Unused Imports
>>>>>>> d. Avoid Star Import
>>>>>>> e. tab character in file
>>>>>>> f. Newline At End Of File
>>>>>>> g. Trailing whitespace found
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Step 2: (241 files, 962 lines (with Step 1))
>>>>>>>
>>>>>>> 1. Set below rules' severity to error in Checkstyle, all rules are
>>> import to make sure the code is correct
>>>>>>> a. Need Braces
>>>>>>> b. Equals Hash Code
>>>>>>> c. Missing Switch Default
>>>>>>> d. Fall Through
>>>>>>> e. No Finalizer
>>>>>>>
>>>>>>>
>>>>>>> 2. Set below rules' severity to error in Checkstyle, all rules are
>>> about name
>>>>>>> a. Member Name
>>>>>>> b. Constant Name
>>>>>>> c. Parameter Name
>>>>>>> d. LambdaParameterName
>>>>>>> e. Local Variable Name
>>>>>>> f. Class Type Parameter Name
>>>>>>> g. Static Variable Name
>>>>>>> h. Method Name
>>>>>>> j. Abbreviation As Word In Name
>>>>>>>
>>>>>>>
>>>>>>> Step 3: (605 files, 6008 lines (with Step 1 and Step 2))
>>>>>>>
>>>>>>> 1. Set below rules' severity to error in Checkstyle, all rules are
>>> about whitespace
>>>>>>> a. Empty Block
>>>>>>> b. Left Curly
>>>>>>> c. Right Curly
>>>>>>> d. White space Around
>>>>>>> e. Generic Whitespace
>>>>>>> f. Indentation
>>>>>>> g. No Whitespace Before Case Default Colon
>>>>>>> h. Method Param Pad
>>>>>>> i. Whitespace
>>>>>>> j. Paren Pad
>>>>>>> k. Extra newline
>>>>>>> l. Extra newline at end of block
>>>>>>> m. Empty Statement
>>>>>>> n. Empty Catch Block
>>>>>>> o. Comments Indentation
>>>>>>>
>>>>>>> 2. Set below rules' severity to error in Checkstyle, all rules are
>>> about the position of the newline
>>>>>>> a. Line Length 120
>>>>>>> b. Operator Wrap
>>>>>>> c. One Statement Per Line
>>>>>>> d. Multiple Variable Declarations
>>>>>>>
>>>>>>>
>>>>>>> Step 4: (674 files, 6730 lines (with Step 1, Step 2 and Step 3))
>>>>>>>
>>>>>>> 1. Set below rules' severity to error in Checkstyle
>>>>>>> a. Array Type Style
>>>>>>> b. Upper Ell
>>>>>>> c. Modifier Order
>>>>>>> d. Empty Line Separator
>>>>>>> e. Separator Wrap
>>>>>>> f. Overload Methods Declaration Order
>>>>>>> g. Variable Declaration Usage Distance
>>>>>>> i. One Top Level Class
>>>>>>>
>>>>>>>
>>>>>>> Step 5:
>>>>>>>
>>>>>>> 1. Add java doc check to CheckStyle and set severity to warning.
>>>>>>>
>>>>>>> 2. Fix java doc warning gradually
>>>>>>>
>>>>>>>
>>>>>>>> 2022年4月28日 00:39，morrysnow <mo...@126.com> 写道：
>>>>>>>>
>>>>>>>> Hi, devs
>>>>>>>>
>>>>>>>> I summarized all the rules I wanted to add. I examined the number of
>>> lines of code affected by all the rules and categorized them by importance.
>>>>>>>>
>>>>>>>> The link is here:
>>> https://github.com/apache/incubator-doris/issues/8985#issuecomment-1111214162
>>> <
>>> https://github.com/apache/incubator-doris/issues/8985#issuecomment-1111214162
>>>>
>>>>>>>>
>>>>>>>> I would like to be able to submit them in multiple patches. The
>>> principle of patch splitting is as follows. Those with low impact and high
>>> importance are submitted first and sets the severity to Error. The Javadoc
>>> commits last and sets the severity to Warning.
>>>>>>>>
>>>>>>>>
>>>>>>>>> 2022年4月18日 16:09，vin jake <ja...@gmail.com> 写道：
>>>>>>>>>
>>>>>>>>> Hi, there is a new PR about format doris.
>>>>>>>>> https://github.com/apache/incubator-doris/pull/9072
>>>>>>>>>
>>>>>>>>> I think it's time to ensure what checkstyle rules we should add
>>> for fe.
>>>>>>>>>
>>>>>>>>> All people who care about format rule can put up your thoughts.
>>>>>>>>>
>>>>>>>>> We can add it in the issue:
>>>>>>>>> https://github.com/apache/incubator-doris/issues/8985
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Thu, Apr 14, 2022 at 6:52 PM vin jake <ja...@gmail.com>
>>> wrote:
>>>>>>>>>
>>>>>>>>>> great, I will trim it in my PR.
>>>>>>>>>>
>>>>>>>>>> checkstyle.xml need more discussion
>>>>>>>>>>
>>>>>>>>>> On Thu, Apr 14, 2022 at 5:48 PM morrysnow <mo...@126.com>
>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> I agree with u.
>>>>>>>>>>>
>>>>>>>>>>> For the first point, I want to list rules first, and then change
>>>>>>>>>>> checkstyle.xml.
>>>>>>>>>>>
>>>>>>>>>>> For the second point, original change information in git is not
>>> lost, we
>>>>>>>>>>> just need to do blame on the version prior to the ‘code style’
>>> commit and
>>>>>>>>>>> some gui tools could list history of one file. Anyway, it is not
>>> very
>>>>>>>>>>> convenience.
>>>>>>>>>>>
>>>>>>>>>>>> 2022年4月14日 17:35，Shuo Wang <wa...@gmail.com> 写道：
>>>>>>>>>>>>
>>>>>>>>>>>> In general, I believe that we should do some work to make the
>>> code clean
>>>>>>>>>>>> and readable.
>>>>>>>>>>>>
>>>>>>>>>>>> My concern is:
>>>>>>>>>>>> 1. We should have an agreement on the code style specification
>>> in the
>>>>>>>>>>>> community at first.
>>>>>>>>>>>> 2. If the many lines of code change after applying the code
>>> style rule,
>>>>>>>>>>> we
>>>>>>>>>>>> would lose the original changelog via `git blame`.
>>>>>>>>>>>>
>>>>>>>>>>>> vin jake <ja...@gmail.com> 于2022年4月14日周四 17:12写道：
>>>>>>>>>>>>
>>>>>>>>>>>>> I have add it in PR
>>>>>>>>>>> https://github.com/apache/incubator-doris/pull/8987
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Thu, Apr 14, 2022 at 4:37 PM morrysnow <mo...@126.com>
>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi, devs,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Currently, we only have two rules in checkstyle.xml in fe.
>>> These are
>>>>>>>>>>> all
>>>>>>>>>>>>>> about import. So, the code style in fe is very casual.
>>>>>>>>>>>>>> I want to add more rules to checkstyle.xml in fe to Improve
>>> code
>>>>>>>>>>>>>> readability, and adjust all fe code to satisfy new code style
>>> step by
>>>>>>>>>>>>> step.
>>>>>>>>>>>>>> What do you think about it? If this is a good idea. I will
>>> research
>>>>>>>>>>> which
>>>>>>>>>>>>>> rules apply to our code and put together a list.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>> ---------------------------------------------------------------------
>>>>>>>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
>>>>>>>>>>>>>> For additional commands, e-mail: dev-help@doris.apache.org
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>> ---------------------------------------------------------------------
>>>>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
>>>>>>>>>>> For additional commands, e-mail: dev-help@doris.apache.org
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> ---------------------------------------------------------------------
>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
>>>>>>> For additional commands, e-mail: dev-help@doris.apache.org
>>>>
>>>
>>>
>>
>> --
>> Ling Miao | Apache Doris
>
Re: Add more rules to checkstyle.xml in fe
Posted by morrysnow <mo...@126.com>.
Hi , all devs
After this PR: https://github.com/apache/incubator-doris/pull/10134 <https://github.com/apache/incubator-doris/pull/10134>, all checkstyle error has been fixed.
So, i want to turn Github workflow 'FE Code Style Checker' to 'Required' after this PR be merged.
After #10134 be merged, when compiling fe with `maven`, `CheckStyle` checks are done by default.
This will slightly slow down compilation. If you want to skip checkstyle, please use the following command to compile
mvn clean install -DskipTests -Dcheckstyle.skip
Also, you can check your code style seperately by run following command under fe directory:
mvn checkstyle:check
> 2022年5月6日 16:16，ling miao <li...@apache.org> 写道：
>
> Regarding the code format correction of the old code, do we currently need
> to do it manually?
>
> Ling Miao
>
> morrysnow <mo...@126.com> 于2022年5月6日周五 15:03写道：
>
>> Hi devs,
>>
>> I create issues for every steps. Any one willing to submit pr could assign
>> issue to himself/herself.
>>
>> Step1: https://github.com/apache/incubator-doris/issues/9403 <
>> https://github.com/apache/incubator-doris/issues/9403>
>> Step2: https://github.com/apache/incubator-doris/issues/9404
>> Step3: https://github.com/apache/incubator-doris/issues/9405
>> Step4: https://github.com/apache/incubator-doris/issues/9406
>>
>>> 2022年4月29日 13:43，morrysnow <mo...@126.com> 写道：
>>>
>>> Hi, devs
>>>
>>> I want to add Step 0 before all steps.
>>>
>>> Step 0:
>>>
>>> Add checkstyle action to github action to check pr style. This action
>> checks incremental code and adds comment to pr automatically.
>>> Action: https://github.com/dbelyaev/action-checkstyle <
>> https://github.com/dbelyaev/action-checkstyle>
>>> Demo: https://github.com/morrySnow/test_checkstyle/pull/5 <
>> https://github.com/morrySnow/test_checkstyle/pull/5>
>>>
>>>> 2022年4月29日 13:40，morrysnow <mo...@126.com> 写道：
>>>>
>>>> Hi, mingyu
>>>>
>>>> 1. Check license is needed for local check.
>>>> 2. Add a rule to forbidden static import is ok for me
>>>>
>>>>> 2022年4月28日 21:34，陈明雨 <mo...@163.com> 写道：
>>>>>
>>>>> That would be great!
>>>>> Some suggestion about the rules:
>>>>>
>>>>>
>>>>>> d. license header check
>>>>> For now, we use skywalking-eyes to check license header, is this rule
>> still needed?
>>>>>
>>>>>
>>>>>> adjust the order of import to "static, org.apache.doris, third party,
>> java"
>>>>> Better not using static import
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>>
>>>>> 此致！Best Regards
>>>>> 陈明雨 Mingyu Chen
>>>>>
>>>>> Email:
>>>>> chenmingyu@apache.org
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> 在 2022-04-28 17:14:34，"morrysnow" <mo...@126.com> 写道：
>>>>>> Hi, devs,
>>>>>>
>>>>>> I split adding java check style into 5 steps. I’m willing to get
>> feedback about it.
>>>>>>
>>>>>> CheckStyle and Spotless will modify about 10k lines.
>>>>>>
>>>>>> Step 1:
>>>>>>
>>>>>> 1. Add Spotless plugin and do some simple format(1198 files changed,
>> 3205 insertions(+), 4350 deletions(-))
>>>>>> a. end with new line
>>>>>> b. trim trailing whitespace
>>>>>> c. replace tab indent with 4 spaces indent
>>>>>> d. license header check
>>>>>> e. adjust the order of import to "static, org.apache.doris, third
>> party, java"
>>>>>> f. remove unused imports
>>>>>> g. make sure all files are encoding by utf-8
>>>>>> h. make sure all files line ending by LF
>>>>>>
>>>>>> 2. Add checkstyle rules but set default severity to warning to avoid
>> compile failed.
>>>>>>
>>>>>> 3. set some rules' severity to error in Checkstyle since no lines in
>> doris break these rules(0 files, 0 lines)
>>>>>> a. Merge conflicts unresolved
>>>>>> b. Avoid using corresponding octal or Unicode escape
>>>>>> c. Avoid Escaped Unicode Characters
>>>>>> d. No Line Wrap
>>>>>> e. Package Name
>>>>>> f. Type Name
>>>>>> g. Annotation Location
>>>>>> h. Interface Type Parameter
>>>>>> i. CatchParameterName
>>>>>> j. Pattern Variable Name
>>>>>> k. Record Component Name
>>>>>> l. Record Type Parameter Name
>>>>>> m. Method Type Parameter Name
>>>>>>
>>>>>> 4. Set below rules severity to error in Checkstyle since these had
>> done by splotless(12 files, 177 lines)
>>>>>> a. Redundant Import
>>>>>> b. Custom Import Order
>>>>>> c. Unused Imports
>>>>>> d. Avoid Star Import
>>>>>> e. tab character in file
>>>>>> f. Newline At End Of File
>>>>>> g. Trailing whitespace found
>>>>>>
>>>>>>
>>>>>>
>>>>>> Step 2: (241 files, 962 lines (with Step 1))
>>>>>>
>>>>>> 1. Set below rules' severity to error in Checkstyle, all rules are
>> import to make sure the code is correct
>>>>>> a. Need Braces
>>>>>> b. Equals Hash Code
>>>>>> c. Missing Switch Default
>>>>>> d. Fall Through
>>>>>> e. No Finalizer
>>>>>>
>>>>>>
>>>>>> 2. Set below rules' severity to error in Checkstyle, all rules are
>> about name
>>>>>> a. Member Name
>>>>>> b. Constant Name
>>>>>> c. Parameter Name
>>>>>> d. LambdaParameterName
>>>>>> e. Local Variable Name
>>>>>> f. Class Type Parameter Name
>>>>>> g. Static Variable Name
>>>>>> h. Method Name
>>>>>> j. Abbreviation As Word In Name
>>>>>>
>>>>>>
>>>>>> Step 3: (605 files, 6008 lines (with Step 1 and Step 2))
>>>>>>
>>>>>> 1. Set below rules' severity to error in Checkstyle, all rules are
>> about whitespace
>>>>>> a. Empty Block
>>>>>> b. Left Curly
>>>>>> c. Right Curly
>>>>>> d. White space Around
>>>>>> e. Generic Whitespace
>>>>>> f. Indentation
>>>>>> g. No Whitespace Before Case Default Colon
>>>>>> h. Method Param Pad
>>>>>> i. Whitespace
>>>>>> j. Paren Pad
>>>>>> k. Extra newline
>>>>>> l. Extra newline at end of block
>>>>>> m. Empty Statement
>>>>>> n. Empty Catch Block
>>>>>> o. Comments Indentation
>>>>>>
>>>>>> 2. Set below rules' severity to error in Checkstyle, all rules are
>> about the position of the newline
>>>>>> a. Line Length 120
>>>>>> b. Operator Wrap
>>>>>> c. One Statement Per Line
>>>>>> d. Multiple Variable Declarations
>>>>>>
>>>>>>
>>>>>> Step 4: (674 files, 6730 lines (with Step 1, Step 2 and Step 3))
>>>>>>
>>>>>> 1. Set below rules' severity to error in Checkstyle
>>>>>> a. Array Type Style
>>>>>> b. Upper Ell
>>>>>> c. Modifier Order
>>>>>> d. Empty Line Separator
>>>>>> e. Separator Wrap
>>>>>> f. Overload Methods Declaration Order
>>>>>> g. Variable Declaration Usage Distance
>>>>>> i. One Top Level Class
>>>>>>
>>>>>>
>>>>>> Step 5:
>>>>>>
>>>>>> 1. Add java doc check to CheckStyle and set severity to warning.
>>>>>>
>>>>>> 2. Fix java doc warning gradually
>>>>>>
>>>>>>
>>>>>>> 2022年4月28日 00:39，morrysnow <mo...@126.com> 写道：
>>>>>>>
>>>>>>> Hi, devs
>>>>>>>
>>>>>>> I summarized all the rules I wanted to add. I examined the number of
>> lines of code affected by all the rules and categorized them by importance.
>>>>>>>
>>>>>>> The link is here:
>> https://github.com/apache/incubator-doris/issues/8985#issuecomment-1111214162
>> <
>> https://github.com/apache/incubator-doris/issues/8985#issuecomment-1111214162
>>>
>>>>>>>
>>>>>>> I would like to be able to submit them in multiple patches. The
>> principle of patch splitting is as follows. Those with low impact and high
>> importance are submitted first and sets the severity to Error. The Javadoc
>> commits last and sets the severity to Warning.
>>>>>>>
>>>>>>>
>>>>>>>> 2022年4月18日 16:09，vin jake <ja...@gmail.com> 写道：
>>>>>>>>
>>>>>>>> Hi, there is a new PR about format doris.
>>>>>>>> https://github.com/apache/incubator-doris/pull/9072
>>>>>>>>
>>>>>>>> I think it's time to ensure what checkstyle rules we should add
>> for fe.
>>>>>>>>
>>>>>>>> All people who care about format rule can put up your thoughts.
>>>>>>>>
>>>>>>>> We can add it in the issue:
>>>>>>>> https://github.com/apache/incubator-doris/issues/8985
>>>>>>>>
>>>>>>>>
>>>>>>>> On Thu, Apr 14, 2022 at 6:52 PM vin jake <ja...@gmail.com>
>> wrote:
>>>>>>>>
>>>>>>>>> great, I will trim it in my PR.
>>>>>>>>>
>>>>>>>>> checkstyle.xml need more discussion
>>>>>>>>>
>>>>>>>>> On Thu, Apr 14, 2022 at 5:48 PM morrysnow <mo...@126.com>
>> wrote:
>>>>>>>>>
>>>>>>>>>> I agree with u.
>>>>>>>>>>
>>>>>>>>>> For the first point, I want to list rules first, and then change
>>>>>>>>>> checkstyle.xml.
>>>>>>>>>>
>>>>>>>>>> For the second point, original change information in git is not
>> lost, we
>>>>>>>>>> just need to do blame on the version prior to the ‘code style’
>> commit and
>>>>>>>>>> some gui tools could list history of one file. Anyway, it is not
>> very
>>>>>>>>>> convenience.
>>>>>>>>>>
>>>>>>>>>>> 2022年4月14日 17:35，Shuo Wang <wa...@gmail.com> 写道：
>>>>>>>>>>>
>>>>>>>>>>> In general, I believe that we should do some work to make the
>> code clean
>>>>>>>>>>> and readable.
>>>>>>>>>>>
>>>>>>>>>>> My concern is:
>>>>>>>>>>> 1. We should have an agreement on the code style specification
>> in the
>>>>>>>>>>> community at first.
>>>>>>>>>>> 2. If the many lines of code change after applying the code
>> style rule,
>>>>>>>>>> we
>>>>>>>>>>> would lose the original changelog via `git blame`.
>>>>>>>>>>>
>>>>>>>>>>> vin jake <ja...@gmail.com> 于2022年4月14日周四 17:12写道：
>>>>>>>>>>>
>>>>>>>>>>>> I have add it in PR
>>>>>>>>>> https://github.com/apache/incubator-doris/pull/8987
>>>>>>>>>>>>
>>>>>>>>>>>> On Thu, Apr 14, 2022 at 4:37 PM morrysnow <mo...@126.com>
>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi, devs,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Currently, we only have two rules in checkstyle.xml in fe.
>> These are
>>>>>>>>>> all
>>>>>>>>>>>>> about import. So, the code style in fe is very casual.
>>>>>>>>>>>>> I want to add more rules to checkstyle.xml in fe to Improve
>> code
>>>>>>>>>>>>> readability, and adjust all fe code to satisfy new code style
>> step by
>>>>>>>>>>>> step.
>>>>>>>>>>>>> What do you think about it? If this is a good idea. I will
>> research
>>>>>>>>>> which
>>>>>>>>>>>>> rules apply to our code and put together a list.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>> ---------------------------------------------------------------------
>>>>>>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
>>>>>>>>>>>>> For additional commands, e-mail: dev-help@doris.apache.org
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>> ---------------------------------------------------------------------
>>>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
>>>>>>>>>> For additional commands, e-mail: dev-help@doris.apache.org
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
>>>>>> For additional commands, e-mail: dev-help@doris.apache.org
>>>
>>
>>
>
> --
> Ling Miao | Apache Doris
Re: Add more rules to checkstyle.xml in fe
Posted by ling miao <li...@apache.org>.
Regarding the code format correction of the old code, do we currently need
to do it manually?
Ling Miao
morrysnow <mo...@126.com> 于2022年5月6日周五 15:03写道：
> Hi devs,
>
> I create issues for every steps. Any one willing to submit pr could assign
> issue to himself/herself.
>
> Step1: https://github.com/apache/incubator-doris/issues/9403 <
> https://github.com/apache/incubator-doris/issues/9403>
> Step2: https://github.com/apache/incubator-doris/issues/9404
> Step3: https://github.com/apache/incubator-doris/issues/9405
> Step4: https://github.com/apache/incubator-doris/issues/9406
>
> > 2022年4月29日 13:43，morrysnow <mo...@126.com> 写道：
> >
> > Hi, devs
> >
> > I want to add Step 0 before all steps.
> >
> > Step 0:
> >
> > Add checkstyle action to github action to check pr style. This action
> checks incremental code and adds comment to pr automatically.
> > Action: https://github.com/dbelyaev/action-checkstyle <
> https://github.com/dbelyaev/action-checkstyle>
> > Demo: https://github.com/morrySnow/test_checkstyle/pull/5 <
> https://github.com/morrySnow/test_checkstyle/pull/5>
> >
> >> 2022年4月29日 13:40，morrysnow <mo...@126.com> 写道：
> >>
> >> Hi, mingyu
> >>
> >> 1. Check license is needed for local check.
> >> 2. Add a rule to forbidden static import is ok for me
> >>
> >>> 2022年4月28日 21:34，陈明雨 <mo...@163.com> 写道：
> >>>
> >>> That would be great!
> >>> Some suggestion about the rules:
> >>>
> >>>
> >>>> d. license header check
> >>> For now, we use skywalking-eyes to check license header, is this rule
> still needed?
> >>>
> >>>
> >>>> adjust the order of import to "static, org.apache.doris, third party,
> java"
> >>> Better not using static import
> >>>
> >>>
> >>>
> >>>
> >>> --
> >>>
> >>> 此致！Best Regards
> >>> 陈明雨 Mingyu Chen
> >>>
> >>> Email:
> >>> chenmingyu@apache.org
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> 在 2022-04-28 17:14:34，"morrysnow" <mo...@126.com> 写道：
> >>>> Hi, devs,
> >>>>
> >>>> I split adding java check style into 5 steps. I’m willing to get
> feedback about it.
> >>>>
> >>>> CheckStyle and Spotless will modify about 10k lines.
> >>>>
> >>>> Step 1:
> >>>>
> >>>> 1. Add Spotless plugin and do some simple format(1198 files changed,
> 3205 insertions(+), 4350 deletions(-))
> >>>> a. end with new line
> >>>> b. trim trailing whitespace
> >>>> c. replace tab indent with 4 spaces indent
> >>>> d. license header check
> >>>> e. adjust the order of import to "static, org.apache.doris, third
> party, java"
> >>>> f. remove unused imports
> >>>> g. make sure all files are encoding by utf-8
> >>>> h. make sure all files line ending by LF
> >>>>
> >>>> 2. Add checkstyle rules but set default severity to warning to avoid
> compile failed.
> >>>>
> >>>> 3. set some rules' severity to error in Checkstyle since no lines in
> doris break these rules(0 files, 0 lines)
> >>>> a. Merge conflicts unresolved
> >>>> b. Avoid using corresponding octal or Unicode escape
> >>>> c. Avoid Escaped Unicode Characters
> >>>> d. No Line Wrap
> >>>> e. Package Name
> >>>> f. Type Name
> >>>> g. Annotation Location
> >>>> h. Interface Type Parameter
> >>>> i. CatchParameterName
> >>>> j. Pattern Variable Name
> >>>> k. Record Component Name
> >>>> l. Record Type Parameter Name
> >>>> m. Method Type Parameter Name
> >>>>
> >>>> 4. Set below rules severity to error in Checkstyle since these had
> done by splotless(12 files, 177 lines)
> >>>> a. Redundant Import
> >>>> b. Custom Import Order
> >>>> c. Unused Imports
> >>>> d. Avoid Star Import
> >>>> e. tab character in file
> >>>> f. Newline At End Of File
> >>>> g. Trailing whitespace found
> >>>>
> >>>>
> >>>>
> >>>> Step 2: (241 files, 962 lines (with Step 1))
> >>>>
> >>>> 1. Set below rules' severity to error in Checkstyle, all rules are
> import to make sure the code is correct
> >>>> a. Need Braces
> >>>> b. Equals Hash Code
> >>>> c. Missing Switch Default
> >>>> d. Fall Through
> >>>> e. No Finalizer
> >>>>
> >>>>
> >>>> 2. Set below rules' severity to error in Checkstyle, all rules are
> about name
> >>>> a. Member Name
> >>>> b. Constant Name
> >>>> c. Parameter Name
> >>>> d. LambdaParameterName
> >>>> e. Local Variable Name
> >>>> f. Class Type Parameter Name
> >>>> g. Static Variable Name
> >>>> h. Method Name
> >>>> j. Abbreviation As Word In Name
> >>>>
> >>>>
> >>>> Step 3: (605 files, 6008 lines (with Step 1 and Step 2))
> >>>>
> >>>> 1. Set below rules' severity to error in Checkstyle, all rules are
> about whitespace
> >>>> a. Empty Block
> >>>> b. Left Curly
> >>>> c. Right Curly
> >>>> d. White space Around
> >>>> e. Generic Whitespace
> >>>> f. Indentation
> >>>> g. No Whitespace Before Case Default Colon
> >>>> h. Method Param Pad
> >>>> i. Whitespace
> >>>> j. Paren Pad
> >>>> k. Extra newline
> >>>> l. Extra newline at end of block
> >>>> m. Empty Statement
> >>>> n. Empty Catch Block
> >>>> o. Comments Indentation
> >>>>
> >>>> 2. Set below rules' severity to error in Checkstyle, all rules are
> about the position of the newline
> >>>> a. Line Length 120
> >>>> b. Operator Wrap
> >>>> c. One Statement Per Line
> >>>> d. Multiple Variable Declarations
> >>>>
> >>>>
> >>>> Step 4: (674 files, 6730 lines (with Step 1, Step 2 and Step 3))
> >>>>
> >>>> 1. Set below rules' severity to error in Checkstyle
> >>>> a. Array Type Style
> >>>> b. Upper Ell
> >>>> c. Modifier Order
> >>>> d. Empty Line Separator
> >>>> e. Separator Wrap
> >>>> f. Overload Methods Declaration Order
> >>>> g. Variable Declaration Usage Distance
> >>>> i. One Top Level Class
> >>>>
> >>>>
> >>>> Step 5:
> >>>>
> >>>> 1. Add java doc check to CheckStyle and set severity to warning.
> >>>>
> >>>> 2. Fix java doc warning gradually
> >>>>
> >>>>
> >>>>> 2022年4月28日 00:39，morrysnow <mo...@126.com> 写道：
> >>>>>
> >>>>> Hi, devs
> >>>>>
> >>>>> I summarized all the rules I wanted to add. I examined the number of
> lines of code affected by all the rules and categorized them by importance.
> >>>>>
> >>>>> The link is here:
> https://github.com/apache/incubator-doris/issues/8985#issuecomment-1111214162
> <
> https://github.com/apache/incubator-doris/issues/8985#issuecomment-1111214162
> >
> >>>>>
> >>>>> I would like to be able to submit them in multiple patches. The
> principle of patch splitting is as follows. Those with low impact and high
> importance are submitted first and sets the severity to Error. The Javadoc
> commits last and sets the severity to Warning.
> >>>>>
> >>>>>
> >>>>>> 2022年4月18日 16:09，vin jake <ja...@gmail.com> 写道：
> >>>>>>
> >>>>>> Hi, there is a new PR about format doris.
> >>>>>> https://github.com/apache/incubator-doris/pull/9072
> >>>>>>
> >>>>>> I think it's time to ensure what checkstyle rules we should add
> for fe.
> >>>>>>
> >>>>>> All people who care about format rule can put up your thoughts.
> >>>>>>
> >>>>>> We can add it in the issue:
> >>>>>> https://github.com/apache/incubator-doris/issues/8985
> >>>>>>
> >>>>>>
> >>>>>> On Thu, Apr 14, 2022 at 6:52 PM vin jake <ja...@gmail.com>
> wrote:
> >>>>>>
> >>>>>>> great, I will trim it in my PR.
> >>>>>>>
> >>>>>>> checkstyle.xml need more discussion
> >>>>>>>
> >>>>>>> On Thu, Apr 14, 2022 at 5:48 PM morrysnow <mo...@126.com>
> wrote:
> >>>>>>>
> >>>>>>>> I agree with u.
> >>>>>>>>
> >>>>>>>> For the first point, I want to list rules first, and then change
> >>>>>>>> checkstyle.xml.
> >>>>>>>>
> >>>>>>>> For the second point, original change information in git is not
> lost, we
> >>>>>>>> just need to do blame on the version prior to the ‘code style’
> commit and
> >>>>>>>> some gui tools could list history of one file. Anyway, it is not
> very
> >>>>>>>> convenience.
> >>>>>>>>
> >>>>>>>>> 2022年4月14日 17:35，Shuo Wang <wa...@gmail.com> 写道：
> >>>>>>>>>
> >>>>>>>>> In general, I believe that we should do some work to make the
> code clean
> >>>>>>>>> and readable.
> >>>>>>>>>
> >>>>>>>>> My concern is:
> >>>>>>>>> 1. We should have an agreement on the code style specification
> in the
> >>>>>>>>> community at first.
> >>>>>>>>> 2. If the many lines of code change after applying the code
> style rule,
> >>>>>>>> we
> >>>>>>>>> would lose the original changelog via `git blame`.
> >>>>>>>>>
> >>>>>>>>> vin jake <ja...@gmail.com> 于2022年4月14日周四 17:12写道：
> >>>>>>>>>
> >>>>>>>>>> I have add it in PR
> >>>>>>>> https://github.com/apache/incubator-doris/pull/8987
> >>>>>>>>>>
> >>>>>>>>>> On Thu, Apr 14, 2022 at 4:37 PM morrysnow <mo...@126.com>
> wrote:
> >>>>>>>>>>
> >>>>>>>>>>> Hi, devs,
> >>>>>>>>>>>
> >>>>>>>>>>> Currently, we only have two rules in checkstyle.xml in fe.
> These are
> >>>>>>>> all
> >>>>>>>>>>> about import. So, the code style in fe is very casual.
> >>>>>>>>>>> I want to add more rules to checkstyle.xml in fe to Improve
> code
> >>>>>>>>>>> readability, and adjust all fe code to satisfy new code style
> step by
> >>>>>>>>>> step.
> >>>>>>>>>>> What do you think about it? If this is a good idea. I will
> research
> >>>>>>>> which
> >>>>>>>>>>> rules apply to our code and put together a list.
> >>>>>>>>>>>
> >>>>>>>>>>>
> ---------------------------------------------------------------------
> >>>>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
> >>>>>>>>>>> For additional commands, e-mail: dev-help@doris.apache.org
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> ---------------------------------------------------------------------
> >>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
> >>>>>>>> For additional commands, e-mail: dev-help@doris.apache.org
> >>>>>>>>
> >>>>>>>>
> >>>>>
> >>>>
> >>>>
> >>>> ---------------------------------------------------------------------
> >>>> To unsubscribe, e-mail: dev-unsubscribe@doris.apache.org
> >>>> For additional commands, e-mail: dev-help@doris.apache.org
> >
>
>
--
Ling Miao | Apache Doris