Ah I see, good references. Perhaps it's really then a committer judgement call on how many changes become "too many" for a single PR. 2016년 5월 15일 (일) 오후 11:16, Hyukjin Kwon <gurwls...@gmail.com>님이 작성:
> Thank you so much for detailed explanation and the history. > > > I understood and it seems *ProcedureDeclarationChecker* should not be > enabled. > > > However, it seems *RedundantIfChecker* okay because there are only two > errors for this across the code base. > > > I have seen some rules have been added time to time, which do not change > super a lot through code base. > > > For example, > > > https://github.com/apache/spark/commit/b0f5497e9520575e5082fa8ce8be5569f43abe74 > > https://github.com/apache/spark/commit/d717ae1fd74d125a9df21350a70e7c2b2d2b4786 > > https://github.com/apache/spark/commit/947b9020b0d621bc97661a0a056297e6889936d3 > > > Thanks! > 2016-05-16 12:05 GMT+09:00 Nicholas Chammas <nicholas.cham...@gmail.com>: > >> Relevant discussion from some time ago: >> https://issues.apache.org/jira/browse/SPARK-3849?focusedCommentId=14168961&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14168961 >> >> In short, if enabling a new style rule requires sweeping changes >> throughout the code base, then it should not be enabled. >> >> We've talked in the past about developing some way of enforcing new style >> rules only on changed lines in a PR, allowing the project's style to >> gradually improve over time without a sudden, sweeping change that breaks >> everybody's workflow. So far nobody's been able to put such a system >> together, as far as I know. >> >> Nick >> >> On Sun, May 15, 2016 at 9:51 PM Hyukjin Kwon <gurwls...@gmail.com> wrote: >> >>> Hi all, >>> >>> Lately, I made a list of rules currently not applied on Spark from >>> http://www.scalastyle.org/rules-dev.html and then I tried to test them. >>> >>> I found two rules that I think might be helpful but I am not too sure. >>> Could I ask both can be added? >>> >>> >>> *RedundantIfChecker *(See >>> http://www.scalastyle.org/rules-dev.html#org_scalastyle_scalariform_RedundantIfChecker >>> ) >>> It seems there are two usage of this. This simply checks if (cond) true >>> else false or if (cond) false else true,which can be just cond or !cond >>> >>> >>> *ProcedureDeclarationChecker *(See >>> http://www.scalastyle.org/rules-dev.html#org_scalastyle_scalariform_ProcedureDeclarationChecker >>> ) >>> >>> >>> It seems this simply checks if functions has the return type `= :Unit` >>> explicitly. This one seems right because it is written in >>> https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide#SparkCodeStyleGuide-ReturnTypes >>> >>> >>> However, it seems the number of occurrence is super a lot. (It seems >>> roughly more than 800 times). It seems this will cause a lot of conflicts. >>> >>> >>> >>> Here is a list of rules not mentioned in scalastyle-config.xml just in >>> case someone wants to know. >>> >>> *IndentationChecker* >>> >>> <check enabled="true" class="org.scalastyle.file.IndentationChecker" >>> level="warning"> >>> >>> <parameters> >>> >>> <parameter name="tabSize">2</parameter> >>> >>> <parameter name="methodParamIndentSize">2</parameter> >>> >>> </parameters> >>> >>> </check> >>> >>> >>> *BlockImportChecker* >>> >>> <check enabled="true" >>> class="org.scalastyle.scalariform.BlockImportChecker" level="warning"/> >>> >>> >>> *DeprecatedJavaChecker* >>> >>> <check enabled="true" >>> class="org.scalastyle.scalariform.DeprecatedJavaChecker" level="warning"/> >>> >>> >>> *EmptyClassChecker* >>> >>> <check enabled="true" >>> class="org.scalastyle.scalariform.EmptyClassChecker" level="warning"/> >>> >>> >>> *ForBraceChecker* >>> >>> <check enabled="true" class="org.scalastyle.scalariform.ForBraceChecker" >>> level="warning"/> >>> >>> >>> *LowercasePatternMatchChecker* >>> >>> <check enabled="true" >>> class="org.scalastyle.scalariform.LowercasePatternMatchChecker" >>> level="warning"/> >>> >>> >>> *MultipleStringLiteralsChecker* >>> >>> <check enabled="true" >>> class="org.scalastyle.scalariform.MultipleStringLiteralsChecker" >>> level="warning"> >>> >>> <parameters> >>> >>> <parameter name="allowed">1</parameter> >>> >>> <parameter name="ignoreRegex">^\"\"$</parameter> >>> >>> </parameters> >>> >>> </check> >>> >>> >>> *PatternMatchAlignChecker* >>> >>> <check enabled="true" >>> class="org.scalastyle.scalariform.PatternMatchAlignChecker" >>> level="warning"/> >>> >>> >>> *ProcedureDeclarationChecker* >>> >>> <check enabled="true" >>> class="org.scalastyle.scalariform.ProcedureDeclarationChecker" >>> level="warning"/> >>> >>> >>> *RedundantIfChecker* >>> >>> <check enabled="true" >>> class="org.scalastyle.scalariform.RedundantIfChecker" level="warning"/> >>> >>> >>> *ScalaDocChecker* >>> >>> <check enabled="true" class="org.scalastyle.scalariform.ScalaDocChecker" >>> level="warning"> >>> >>> <parameters> >>> >>> <parameter name="ignoreRegex">(.*Spec$)|(.*SpecIT$)</parameter> >>> >>> </parameters> >>> >>> </check> >>> >>> >>> *TodoCommentChecker* >>> >>> <checker enabled="true" >>> class="org.scalastyle.scalariform.TodoCommentChecker" level="warning"> >>> >>> <parameters> >>> >>> <parameter default="TODO|FIXME" type="string" name="words"/> >>> >>> </parameters> >>> >>> </checker> >>> >>> >>> *VarFieldChecker* >>> >>> <check enabled="true" class="org.scalastyle.scalariform.VarFieldChecker" >>> level="warning"/> >>> >>> >>> *VarLocalChecker* >>> >>> <check enabled="true" class="org.scalastyle.scalariform.VarLocalChecker" >>> level="warning"/> >>> >>> >>> *WhileChecker* >>> >>> <check enabled="true" class="org.scalastyle.scalariform.WhileChecker" >>> level="warning"/> >>> >>> >>> *XmlLiteralChecker* >>> >>> <check enabled="true" >>> class="org.scalastyle.scalariform.XmlLiteralChecker" level="warning"/> >>> >>> >>> >>> Thank you very much!! >>> >>
