Re: Question about enabling some of missing rules.
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님이 작성: > 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 : > >> Relevant discussion from some time ago: >> https://issues.apache.org/jira/browse/SPARK-3849?focusedCommentId=14168961=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 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* >>> >>> >> level="warning"> >>> >>> >>> >>> 2 >>> >>> 2 >>> >>> >>> >>> >>> >>> >>> *BlockImportChecker* >>> >>> >> class="org.scalastyle.scalariform.BlockImportChecker" level="warning"/> >>> >>> >>> *DeprecatedJavaChecker* >>> >>> >> class="org.scalastyle.scalariform.DeprecatedJavaChecker" level="warning"/> >>> >>> >>> *EmptyClassChecker* >>> >>> >> class="org.scalastyle.scalariform.EmptyClassChecker" level="warning"/> >>> >>> >>> *ForBraceChecker* >>> >>> >> level="warning"/> >>> >>> >>> *LowercasePatternMatchChecker* >>> >>> >> class="org.scalastyle.scalariform.LowercasePatternMatchChecker" >>> level="warning"/> >>> >>> >>> *MultipleStringLiteralsChecker* >>> >>> >> class="org.scalastyle.scalariform.MultipleStringLiteralsChecker" >>> level="warning"> >>> >>> >>> >>> 1 >>> >>> ^\\$ >>> >>> >>> >>> >>> >>> >>> *PatternMatchAlignChecker* >>> >>> >> class="org.scalastyle.scalariform.PatternMatchAlignChecker" >>> level="warning"/> >>> >>> >>> *ProcedureDeclarationChecker* >>> >>> >> class="org.scalastyle.scalariform.ProcedureDeclarationChecker" >>> level="warning"/> >>> >>> >>> *RedundantIfChecker* >>> >>> >> class="org.scalastyle.scalariform.RedundantIfChecker" level="warning"/> >>> >>> >>> *ScalaDocChecker* >>> >>> >> level="warning"> >>> >>> >>> >>> (.*Spec$)|(.*SpecIT$) >>> >>> >>> >>> >>> >>> >>> *TodoCommentChecker* >>> >>> >> class="org.scalastyle.scalariform.TodoCommentChecker" level="warning"> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> *VarFieldChecker* >>> >>> >> level="warning"/> >>> >>> >>> *VarLocalChecker* >>> >>> >> level="warning"/> >>> >>> >>> *WhileChecker* >>> >>> >> level="warning"/> >>> >>> >>> *XmlLiteralChecker* >>> >>> >> class="org.scalastyle.scalariform.XmlLiteralChecker" level="warning"/> >>> >>> >>> >>> Thank you very much!! >>> >>
Re: Question about enabling some of missing rules.
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: > Relevant discussion from some time ago: > https://issues.apache.org/jira/browse/SPARK-3849?focusedCommentId=14168961=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 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* >> >> > level="warning"> >> >> >> >> 2 >> >> 2 >> >> >> >> >> >> >> *BlockImportChecker* >> >> > class="org.scalastyle.scalariform.BlockImportChecker" level="warning"/> >> >> >> *DeprecatedJavaChecker* >> >> > class="org.scalastyle.scalariform.DeprecatedJavaChecker" level="warning"/> >> >> >> *EmptyClassChecker* >> >> > class="org.scalastyle.scalariform.EmptyClassChecker" level="warning"/> >> >> >> *ForBraceChecker* >> >> > level="warning"/> >> >> >> *LowercasePatternMatchChecker* >> >> > class="org.scalastyle.scalariform.LowercasePatternMatchChecker" >> level="warning"/> >> >> >> *MultipleStringLiteralsChecker* >> >> > class="org.scalastyle.scalariform.MultipleStringLiteralsChecker" >> level="warning"> >> >> >> >> 1 >> >> ^\\$ >> >> >> >> >> >> >> *PatternMatchAlignChecker* >> >> > class="org.scalastyle.scalariform.PatternMatchAlignChecker" >> level="warning"/> >> >> >> *ProcedureDeclarationChecker* >> >> > class="org.scalastyle.scalariform.ProcedureDeclarationChecker" >> level="warning"/> >> >> >> *RedundantIfChecker* >> >> > class="org.scalastyle.scalariform.RedundantIfChecker" level="warning"/> >> >> >> *ScalaDocChecker* >> >> > level="warning"> >> >> >> >> (.*Spec$)|(.*SpecIT$) >> >> >> >> >> >> >> *TodoCommentChecker* >> >> > class="org.scalastyle.scalariform.TodoCommentChecker" level="warning"> >> >> >> >> >> >> >> >> >> >> >> *VarFieldChecker* >> >> > level="warning"/> >> >> >> *VarLocalChecker* >> >> > level="warning"/> >> >> >> *WhileChecker* >> >> > level="warning"/> >> >> >> *XmlLiteralChecker* >> >> > class="org.scalastyle.scalariform.XmlLiteralChecker" level="warning"/> >> >> >> >> Thank you very much!! >> >
Re: Question about enabling some of missing rules.
Relevant discussion from some time ago: https://issues.apache.org/jira/browse/SPARK-3849?focusedCommentId=14168961=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 Kwonwrote: > 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* > > level="warning"> > > > > 2 > > 2 > > > > > > > *BlockImportChecker* > > class="org.scalastyle.scalariform.BlockImportChecker" level="warning"/> > > > *DeprecatedJavaChecker* > > class="org.scalastyle.scalariform.DeprecatedJavaChecker" level="warning"/> > > > *EmptyClassChecker* > > level="warning"/> > > > *ForBraceChecker* > > level="warning"/> > > > *LowercasePatternMatchChecker* > > class="org.scalastyle.scalariform.LowercasePatternMatchChecker" > level="warning"/> > > > *MultipleStringLiteralsChecker* > > class="org.scalastyle.scalariform.MultipleStringLiteralsChecker" > level="warning"> > > > > 1 > > ^\\$ > > > > > > > *PatternMatchAlignChecker* > > class="org.scalastyle.scalariform.PatternMatchAlignChecker" > level="warning"/> > > > *ProcedureDeclarationChecker* > > class="org.scalastyle.scalariform.ProcedureDeclarationChecker" > level="warning"/> > > > *RedundantIfChecker* > > class="org.scalastyle.scalariform.RedundantIfChecker" level="warning"/> > > > *ScalaDocChecker* > > level="warning"> > > > > (.*Spec$)|(.*SpecIT$) > > > > > > > *TodoCommentChecker* > > class="org.scalastyle.scalariform.TodoCommentChecker" level="warning"> > > > > > > > > > > > *VarFieldChecker* > > level="warning"/> > > > *VarLocalChecker* > > level="warning"/> > > > *WhileChecker* > > level="warning"/> > > > *XmlLiteralChecker* > > level="warning"/> > > > > Thank you very much!! >
Question about enabling some of missing rules.
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* 2 2 *BlockImportChecker* *DeprecatedJavaChecker* *EmptyClassChecker* *ForBraceChecker* *LowercasePatternMatchChecker* *MultipleStringLiteralsChecker* 1 ^\\$ *PatternMatchAlignChecker* *ProcedureDeclarationChecker* *RedundantIfChecker* *ScalaDocChecker* (.*Spec$)|(.*SpecIT$) *TodoCommentChecker* *VarFieldChecker* *VarLocalChecker* *WhileChecker* *XmlLiteralChecker* Thank you very much!!