Improve my car... ehh... sbt #16

sideeffffect · 2019-09-24T18:20:03Z

jakubjanecek

I do not agree with all the changes. To me it seems that the build got more complicated than it was before and the benefits are not obvious. I am also afraid of false positives in the linters etc. Anyway, please update the PR and we'll see.

.travis.yml

build.sbt

jakubjanecek · 2019-09-24T18:41:12Z

build.sbt

+    "-P:silencer:checkUnused"
+  ),
+  scalacOptions --= {
+    if (!sys.env.contains("TRAVIS"))


Why? I'd like to keep the build as clean as possible from such ifs.

I think it's worth failing warnings only on CI, because locally fatal warnings doesn't work well with scalafix, eg https://scalacenter.github.io/scalafix/docs/rules/RemoveUnused.html

And this is exactly what I was talking about! The linters are not mature in my opinion because they have these limitations I don't like. I want my build to fail on warnings locally the same way it fails in CI. It's not acceptable for me to have it behave differently. I'd rather get rid of scalafix than this, sorry.

I think what you really want it linters to only warn locally (because it's very annoying if you have some work-in-progress and it fails because of unused import), but also have an easy way how to turn them fatal.

build.sbt

example/src/main/scala/com/avast/server/toolkit/example/Main.scala

project/Dependencies.scala

# Conflicts: # build.sbt # example/src/main/scala/com/avast/server/toolkit/example/Main.scala # project/Dependencies.scala

sideeffffect · 2019-09-24T21:01:50Z

@jakubjanecek build is green, ready to merge from my POV. we can talk about details in person tomorrow if you want

implemented suggestions

tomasherman · 2019-09-25T07:05:59Z

example/lock.sbt

+// DON'T EDIT THIS FILE.
+// This file is auto generated by sbt-lock 0.6.1.
+// https://github.com/tkawachi/sbt-lock/
+dependencyOverrides ++= {


i don't like this at all - what problem does it solve?

I also think right now we are not in a state where we would need this. We can always add it but I'd like to remove it for now too.

let's not worry about lock right now. removed.

tomasherman · 2019-09-25T07:07:07Z

jvm-execution/src/main/scala/com/avast/server/toolkit/execution/ConfigurableThreadFactory.scala

@@ -35,6 +35,7 @@ object ConfigurableThreadFactory {
  /**
    * @param nameFormat Formatted with long number, e.g. my-thread-%02d
    */
+  @SuppressWarnings(Array("org.wartremover.warts.DefaultArguments"))


why are we calling DefaultArguments wart? if it's a wart, why are we using it?

@sideeffffect Can you explain the wart? I am using default arguments on purpose and having to suppress it everywhere in my code seems very strange, I'd rather remove the wart.

default arguments are frowned upon in the FP community, I personally avoid them too
see https://blog.ssanj.net/posts/2019-05-01-why-are-default-parameter-values-considered-bad-in-scala.html
but this wart applies also to constructors too, which is a big part of PureConfig functionality
so I'm disabling the wart

build.sbt

.scalafix.conf

example/src/main/scala/com/avast/server/toolkit/example/Main.scala

.travis.yml

jakubjanecek · 2019-09-25T07:36:56Z

@hanny24 @augi Inviting you to the discussion because there seems to be quite big disagreement on how build should look. I am interested in your opinion on the setup of linters (scalafix, wart remover) and use of extra compiler plugins (silencer) My view is that for now it seems to just complicate the build and the code (forcing us to suppress warnings where we shouldn't have) and it does not give us much now - it would protect us probably from some trouble in the future but now I think I am a better linter myself :)

augi

In general, it's strange to me to have so much infrastructure code to setup this relatively simple project.

augi · 2019-09-24T20:48:16Z

project/Dependencies.scala

-  val zioInteropCats = "dev.zio" %% "zio-interop-cats" % "2.0.0.0-RC4"
+  object Versions {
+    val catsEffect = "2.0.0"
+    val kindProjector = "0.10.3"


I'm just curious if Scala Steward would be OK with this style.

it is - see here: https://github.com/avast/datadog4s/pull/43/files

Yes, this needs to work with Steward, good point.

No, the point is that the versions are now refactored into separate Version object and I would be afraid it does not work that way...

yeah that might be a problem

it does
https://github.com/typelevel/frameless/pull/397/files

example/src/main/scala/com/avast/server/toolkit/example/Main.scala

augi · 2019-09-25T07:41:17Z

jvm-execution/src/main/scala/com/avast/server/toolkit/execution/ThreadPoolExecutorConfig.scala

@@ -4,6 +4,7 @@ import java.util.concurrent.TimeUnit

 import scala.concurrent.duration.{Duration, FiniteDuration}

+@SuppressWarnings(Array("org.wartremover.warts.DefaultArguments"))


We definitely want to remove this wart.

hanny24

I think that this PR combines many things into one which makes it very difficult to argue about. I think it's very premature to use sbt lock right now. It will just complicate things right now. But it might be useful once we reach 1.0 version.

On the other hand scalafix might be useful right away. I would just be very careful about the rules that we use. What I would start with right now is some sort of rule that makes sure the imports are properly organized.

jakubjanecek · 2019-09-25T10:14:57Z

@sideeffffect I think @hanny24 is right about the fact that by redoing the whole build it triggered a lot of alarms. I know you have put a lot of work into this and you mean it well however as you can see different people have different opinions so as a whole I don't think we can merge this. I suggest we close this PR and try to add some of the things one by one so that we can digest it slowly. The things I think can be done are the following:

Adding scalafix but with less rules (the ones that are not controversial and work well across the codebase.
Marking test dependencies at the place of use. However please keep the versions inlined and do not refactor them to a special object (there's really no need for that, at least now).
Removing the SCM info.
Maybe adding some warts but definitely not all of them like now.

And that's it. Let's start with this and we'll update in the future according to our needs. What do you say?

# Conflicts: # build.sbt # project/Dependencies.scala

* no lock * default arguments allowed

sideeffffect

implemented the requested changes

.scalafix.conf

sideeffffect · 2019-10-01T18:35:09Z

example/lock.sbt

+// DON'T EDIT THIS FILE.
+// This file is auto generated by sbt-lock 0.6.1.
+// https://github.com/tkawachi/sbt-lock/
+dependencyOverrides ++= {


let's not worry about lock right now. removed.

example/src/main/scala/com/avast/server/toolkit/example/Main.scala

sideeffffect · 2019-10-01T18:39:27Z

jvm-execution/src/main/scala/com/avast/server/toolkit/execution/ConfigurableThreadFactory.scala

@@ -35,6 +35,7 @@ object ConfigurableThreadFactory {
  /**
    * @param nameFormat Formatted with long number, e.g. my-thread-%02d
    */
+  @SuppressWarnings(Array("org.wartremover.warts.DefaultArguments"))


default arguments are frowned upon in the FP community, I personally avoid them too
see https://blog.ssanj.net/posts/2019-05-01-why-are-default-parameter-values-considered-bad-in-scala.html
but this wart applies also to constructors too, which is a big part of PureConfig functionality
so I'm disabling the wart

sideeffffect · 2019-10-01T18:40:12Z

jvm-execution/src/main/scala/com/avast/server/toolkit/execution/ThreadPoolExecutorConfig.scala

@@ -4,6 +4,7 @@ import java.util.concurrent.TimeUnit

 import scala.concurrent.duration.{Duration, FiniteDuration}

+@SuppressWarnings(Array("org.wartremover.warts.DefaultArguments"))


sideeffffect · 2019-10-01T18:44:08Z

project/Dependencies.scala

-  val zioInteropCats = "dev.zio" %% "zio-interop-cats" % "2.0.0.0-RC4"
+  object Versions {
+    val catsEffect = "2.0.0"
+    val kindProjector = "0.10.3"


it does
https://github.com/typelevel/frameless/pull/397/files

jakubjanecek · 2019-10-02T10:55:01Z

.scalafix.conf

+  "scala/Any"
+  ## "scala/Product" # triggers even on non-synthetic
+
+  # local type inference + covariant types fires this


Could you please remove this as it is commented anyway?

jakubjanecek · 2019-10-02T10:56:57Z

build.sbt


 ThisBuild / turbo := true

-lazy val commonSettings = Seq(
+lazy val commonSettings = BuildHelper.settingsCommon ++ Seq(


Does it make sense to have commonSettings and BuildHelper.settingsCommon? I'd prefer to have object called BuildSettings and have common or commonSettings inside with everything.

I think this is what we want. BuildHelper.settingsCommon contains common settings for Avast Scala projects. commonSettings contains common settings specific for scala-server-toolkit modules only. Those 2 are not necessarily the same thing.
BuildHelper.settingsCommon for example doesn't have any dependency on cats or scalatest, and it shouldn't.

example/src/main/scala/com/avast/server/toolkit/example/Main.scala

jakubjanecek · 2019-10-02T12:24:52Z

project/BuildHelper.scala

+      "-P:silencer:checkUnused"
+    ),
+    scalacOptions --= {
+      if (!sys.env.contains("TRAVIS"))


Please remove this, I don't think it's needed for now.

I think this is a good idea to fail on warnings in CI
locally, you don't want to fail on warnings, because it break scalafix workflow unfortunatelly (see the comment in the file below)

jakubjanecek

Thank you for your effort and sorry for being so strict ;)

PR changed according to our comments.

pimp my sbt

3a90e5d

jakubjanecek previously requested changes Sep 24, 2019

View reviewed changes

jakubjanecek self-assigned this Sep 24, 2019

sideeffffect added 4 commits September 24, 2019 20:53

Merge remote-tracking branch 'upstream/master' into pimp-my-sbt

e958b10

# Conflicts: # build.sbt # example/src/main/scala/com/avast/server/toolkit/example/Main.scala # project/Dependencies.scala

merge fixes

f9ea866

implement suggestions from review

f91edca

unused imports

d34ce31

sideeffffect changed the title ~~pimp my sbt~~ Improve my car... ehh... sbt Sep 24, 2019

tomasherman reviewed Sep 25, 2019

View reviewed changes

build.sbt Outdated Show resolved Hide resolved

jakubjanecek requested changes Sep 25, 2019

View reviewed changes

.scalafix.conf Outdated Show resolved Hide resolved

example/src/main/scala/com/avast/server/toolkit/example/Main.scala Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

jakubjanecek requested review from hanny24 and augi September 25, 2019 07:33

augi previously requested changes Sep 25, 2019

View reviewed changes

hanny24 previously requested changes Sep 25, 2019

View reviewed changes

jakubjanecek closed this Sep 27, 2019

sideeffffect added 2 commits October 1, 2019 20:25

Merge remote-tracking branch 'upstream/master' into pimp-my-sbt

3518d7e

# Conflicts: # build.sbt # project/Dependencies.scala

changes base on review, mainly:

5768eae

* no lock * default arguments allowed

sideeffffect commented Oct 1, 2019

View reviewed changes

sideeffffect reopened this Oct 1, 2019

sideeffffect added 3 commits October 1, 2019 21:04

revert travis type push condition

9c04d5d

coursier cache

e3fac64

bring back scala.language.higherKinds

8bd18e3

sideeffffect force-pushed the pimp-my-sbt branch from 55bdefb to 8bd18e3 Compare October 1, 2019 19:16

silencer 1.4.4

7b0e6cb

sideeffffect requested review from hanny24, augi and jakubjanecek October 1, 2019 19:39

remove unnecessary wartremover suppression

fa6e26a

jakubjanecek requested changes Oct 2, 2019

View reviewed changes

sideeffffect added 3 commits October 2, 2019 14:51

implement suggestions from review

159a648

implement suggestions from review

22f949c

-Xfatal-warnings always

f3a3ede

jakubjanecek approved these changes Oct 2, 2019

View reviewed changes

jakubjanecek merged commit ec97702 into avast:master Oct 2, 2019

sideeffffect deleted the pimp-my-sbt branch October 2, 2019 14:07

jakubjanecek added the feature New feature or request label Oct 15, 2019

		@@ -4,6 +4,7 @@ import java.util.concurrent.TimeUnit

		import scala.concurrent.duration.{Duration, FiniteDuration}

		@SuppressWarnings(Array("org.wartremover.warts.DefaultArguments"))

Improve my car... ehh... sbt #16

Improve my car... ehh... sbt #16

Uh oh!

Conversation

sideeffffect commented Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakubjanecek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sideeffffect commented Sep 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jakubjanecek commented Sep 25, 2019

Uh oh!

augi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanny24 left a comment

Choose a reason for hiding this comment

Uh oh!

jakubjanecek commented Sep 25, 2019

Uh oh!

sideeffffect left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

sideeffffect commented Sep 24, 2019 •

edited

Loading

sideeffffect Oct 2, 2019 •

edited

Loading