#622 Make deprecations and feature flag more explicit in compile output and addressing some boring warnings

[fmasion]

Thanks for your pull request. Please review the following guidelines.

• Title includes issue id.
• Description of the change added.
• Commits are squashed.
• Tests added.
• Documentation added/updated.
• Also please review CONTRIBUTING.md.

[anilgursel]

I am not sure if 30.secondsor 30 seconds (with import scala.language.postfixOps) preferable.

[fmasion]

I agree 30 seconds is elegant but 30.seconds is less difficult for the compiler to figure out
Postfix operator notation is unsafe and should not be used.
see here Scala style recommendation

The new default is to emit warnings when used
The import is only a way to stop these warnings because postfix is massively used in DSLs

The same applies to :
functor map function
the prefered version is functor.map(function)

[sebady]

Since the scala style guide recommendation is clear here, I would vote we move away from Postfix. I think for test code though postfix improves readability in some cases and I would prefer to optionally continue to use it there.

[fmasion]

@sebady tests, by nature, are DLSs :)
the magic of DSL is built upon implicits, postfix and magnet pattern

Don't conclude that postfix is evil
It's only a matter of balance between readability and safety

IMHO 30.seconds is still very readable and makes compiler faster ;)

[akara]

Sorry for chiming in late. Agree with 30.seconds.

[akara]

I'm good with pretty much all the changes. The use of higherKinds and implicitConversion is well placed. The only sticky one is the use of existentials. I'm still wondering whether that is well placed. Just need an explanation. Thanks so much for doing this work.

[akara]

I'm wondering about this liberal import scala.language.existentials. I'm not clear we're even using existential types in the code block below.

[fmasion]

Honestly I'm not quite sure how to resolve this.
Here's the compiler output with -feature flag activated

[info] Compiling 1 Scala source to /Users/Hit/Dev/squbs/squbs-ext/target/scala-2.12/classes ...
[warn] /Users/Hit/Dev/squbs/squbs-ext/src/main/scala/org/squbs/resolver/Resolver.scala:70:32: inferred existential type (Class[_$1], org.squbs.resolver.Resolver[T]) forSome { type _$1 }, which cannot be expressed by wildcards,  should be enabled
[warn] by making the implicit value scala.language.existentials visible.
[warn] This can be achieved by adding the import clause 'import scala.language.existentials'
[warn] or by setting the compiler option -language:existentials.
[warn] See the Scaladoc for value scala.language.existentials for a discussion
[warn] why the feature should be explicitly enabled.
[warn]       case None => resolvers = (classTag[T].runtimeClass, resolver) :: resolvers
[warn]                                ^

I presume it's a warning because the code uses scala.reflect.classTag that makes the type only known at runtime ?
If you know how to fix this I'll do the change
I tried to reduce the "scope" of the import inside the function
this import is only a feature flag to tell the compiler "I know what I do don't warn me"

[akara]

Seems like a pretty common issue with Class[_]. Lets take it as in your current PR then.

[akara]

Ditto on these.

[fmasion]

here the output for this one :

[warn] /Users/Hit/Dev/squbs/squbs-ext/src/main/scala/org/squbs/util/ConfigUtil.scala:129:9: inferred existential type Option[java.util.List[?0]] forSome { type ?0 <: com.typesafe.config.Config }, which cannot be expressed by wildcards,  should be enabled
[warn] by making the implicit value scala.language.existentials visible.
[warn] This can be achieved by adding the import clause 'import scala.language.existentials'
[warn] or by setting the compiler option -language:existentials.
[warn] See the Scaladoc for value scala.language.existentials for a discussion
[warn] why the feature should be explicitly enabled.
[warn]         try {
[warn]         ^
[warn] one warning found

the signature of underlying.getConfigList(path) is
List<? extends Config> getConfigList(String path); in java
that the scala compiler translates to :
java.util.List[?0]] forSome { type ?0 <: com.typesafe.config.Config}
subtypes of Config

We can remove the warning here by explicitly casting the java result like this :

    def getOptionalConfigList(path: String): Option[Seq[Config]] = {
      val list =
        try {
          Some(underlying.getConfigList(path).asScala.map(x=> x:Config)) //CAST HERE
        } catch {
          case e: ConfigException.Missing => None
        }
      list map (_.toSeq)
    }

Or by importing the

import scala.language.existentials

What do you prefer ?

[akara]

I'd be fine if it is a single cast with .asInstanceOf but not if we need to cast record by record. Please see whether that works. If not I'll accept it as is.

[fmasion]

even better :

    def getOptionalConfigList(path: String): Option[Seq[Config]] = try {
          Some(underlying.getConfigList(path).asScala)
        } catch {
          case e: ConfigException.Missing => None
        }

works without warning
javaConverters may do the job ?

[fmasion]

@akara updated and rebased

[akara]

LGTM