Re: Enforcing scalafmt on Spark Connect - connector/connect
I have prepared the following pull request that enforces scalafmt on the Spark Connect module. Please feel free to have a look and leave comments. If we're reaching consensus on the decision, I will take care of pushing a PR as well for the previously mentioned website to add a small note on using `dev/lint-scala` for local style checks. Thanks Martin On Fri, Oct 14, 2022 at 11:09 AM Yikun Jiang wrote: > +1, I also think it's a good idea. > > BTW, we might also consider adding some notes about `lint-scala` in [1], > just like `lint-python` in pyspark [2]. > > [1] https://spark.apache.org/developer-tools.html > [2] > https://spark.apache.org/docs/latest/api/python/development/contributing.html > > > Regards, > Yikun > > > On Fri, Oct 14, 2022 at 4:51 PM Hyukjin Kwon wrote: > >> I personally like this idea. At least we now do this in PySpark, and it's >> pretty nice that you can just forget about formatting it manually by >> yourself. >> >> On Fri, 14 Oct 2022 at 16:37, Martin Grund >> wrote: >> >>> Hi folks, >>> >>> I'm reaching out to ask to gather input / consensus on the following >>> proposal: Since Spark Connect is effectively new code, I would like to >>> enforce scalafmt explicitly *only* on this module by adding a check in >>> `dev/lint-scala` that checks if there is a diff after running >>> >>> ./build/mvn -Pscala-2.12 scalafmt:format -Dscalafmt.skip=false -pl >>> connector/connect >>> >>> I know that enforcing scalafmt is not desirable on the existing code >>> base but since the Spark Connect code is very new I'm thinking it might >>> reduce friction in the code reviews and create a consistent style. >>> >>> In my previous code reviews where I have applied scalafmt I've >>> received feedback on the import grouping that scalafmt is changing >>> different from our default style. I've prepared a PR >>> https://github.com/apache/spark/pull/38252 to address this issue by >>> explicitly setting it in the scalafmt option. >>> >>> Would you be supportive of enforcing scalafmt *only* on the Spark >>> Connect module? >>> >>> Thanks >>> Martin >>> >>
Re: Enforcing scalafmt on Spark Connect - connector/connect
+1, I also think it's a good idea. BTW, we might also consider adding some notes about `lint-scala` in [1], just like `lint-python` in pyspark [2]. [1] https://spark.apache.org/developer-tools.html [2] https://spark.apache.org/docs/latest/api/python/development/contributing.html Regards, Yikun On Fri, Oct 14, 2022 at 4:51 PM Hyukjin Kwon wrote: > I personally like this idea. At least we now do this in PySpark, and it's > pretty nice that you can just forget about formatting it manually by > yourself. > > On Fri, 14 Oct 2022 at 16:37, Martin Grund > wrote: > >> Hi folks, >> >> I'm reaching out to ask to gather input / consensus on the following >> proposal: Since Spark Connect is effectively new code, I would like to >> enforce scalafmt explicitly *only* on this module by adding a check in >> `dev/lint-scala` that checks if there is a diff after running >> >> ./build/mvn -Pscala-2.12 scalafmt:format -Dscalafmt.skip=false -pl >> connector/connect >> >> I know that enforcing scalafmt is not desirable on the existing code base >> but since the Spark Connect code is very new I'm thinking it might reduce >> friction in the code reviews and create a consistent style. >> >> In my previous code reviews where I have applied scalafmt I've >> received feedback on the import grouping that scalafmt is changing >> different from our default style. I've prepared a PR >> https://github.com/apache/spark/pull/38252 to address this issue by >> explicitly setting it in the scalafmt option. >> >> Would you be supportive of enforcing scalafmt *only* on the Spark >> Connect module? >> >> Thanks >> Martin >> >
Re: Enforcing scalafmt on Spark Connect - connector/connect
I personally like this idea. At least we now do this in PySpark, and it's pretty nice that you can just forget about formatting it manually by yourself. On Fri, 14 Oct 2022 at 16:37, Martin Grund wrote: > Hi folks, > > I'm reaching out to ask to gather input / consensus on the following > proposal: Since Spark Connect is effectively new code, I would like to > enforce scalafmt explicitly *only* on this module by adding a check in > `dev/lint-scala` that checks if there is a diff after running > > ./build/mvn -Pscala-2.12 scalafmt:format -Dscalafmt.skip=false -pl > connector/connect > > I know that enforcing scalafmt is not desirable on the existing code base > but since the Spark Connect code is very new I'm thinking it might reduce > friction in the code reviews and create a consistent style. > > In my previous code reviews where I have applied scalafmt I've > received feedback on the import grouping that scalafmt is changing > different from our default style. I've prepared a PR > https://github.com/apache/spark/pull/38252 to address this issue by > explicitly setting it in the scalafmt option. > > Would you be supportive of enforcing scalafmt *only* on the Spark Connect > module? > > Thanks > Martin >
Enforcing scalafmt on Spark Connect - connector/connect
Hi folks, I'm reaching out to ask to gather input / consensus on the following proposal: Since Spark Connect is effectively new code, I would like to enforce scalafmt explicitly *only* on this module by adding a check in `dev/lint-scala` that checks if there is a diff after running ./build/mvn -Pscala-2.12 scalafmt:format -Dscalafmt.skip=false -pl connector/connect I know that enforcing scalafmt is not desirable on the existing code base but since the Spark Connect code is very new I'm thinking it might reduce friction in the code reviews and create a consistent style. In my previous code reviews where I have applied scalafmt I've received feedback on the import grouping that scalafmt is changing different from our default style. I've prepared a PR https://github.com/apache/spark/pull/38252 to address this issue by explicitly setting it in the scalafmt option. Would you be supportive of enforcing scalafmt *only* on the Spark Connect module? Thanks Martin
