bootstrap/nailgun minor cleanup #41
Conversation
| path = Paths.get(target+file+".jar"); | ||
| try { url = new URL("https://repo1.maven.org/maven2/org/scala-lang/"+folder+"/"+file+".jar"); } | ||
| catch (final MalformedURLException e) { throw new RuntimeException(e); } | ||
| hash = hash; |
There was a problem hiding this comment.
This was a bug; it was simply reassigning the argument to itself and the hash member remained null. Marking the fields as final exposed this.
| } | ||
|
|
||
| // scala-lang dependency | ||
| public static Dependency scala(String target, String scalaVersion, String scalaModule, String hash) |
There was a problem hiding this comment.
I'm a bit bothered - maybe for no good reason - that instantiating a Java dependency requires new Dependency and instantiating a Scala dependency is Dependency.scala. Another option would be to protect the constructor and add Dependency.java so our interface is symmetrical.
That has little to no practical effect, I think, so if you hate it then feel free to ignore me. My feelings will not be hurt. :)
There was a problem hiding this comment.
In case you missed that. bootstrap_scala is tiny a self-contained scala jar downloader, that integrates with the rest of cbt via processes, not via the jvm. It never ends up on the same classpath as CBT itself. So I don't think the internal API of bootstrap_scala matters much.
(We tried but failed at recording the architecture session at NEScala. I'll try to do a screen cast one of these days.)
|
Didn't check it out and test it, but it looks good on scan. :) |
| import java.nio.file.StandardCopyOption; | ||
| import java.security.MessageDigest; | ||
| import java.security.NoSuchAlgorithmException; | ||
| import java.util.Arrays; |
There was a problem hiding this comment.
I am generally a fan of wild-card imports, because I don't see the value of spending time micro-managing imports and I don't use an IDE that does it for me. I very rarely had bugs because of that. When I did, it was with the areas where Scala's type-safetly is lacking, in particular Any2StringAdd. I think I'd rather try to enforce using ++ everywhere than micromanaging imports, especially when they are all JDK ones. WDYT?
There was a problem hiding this comment.
I use wildcard imports when I'm trying to figure out what I'm doing, but I usually turn them into explicit imports as part of general tidying up. To me it's just another source of confusion that I can minimize with a minute or two of work. I don't use an IDE either, so the imports help me figure out where things are coming from.
But you're driving the bus so I can revert this to wildcards if you like.
There was a problem hiding this comment.
na, let's not waste time on going back. The only way is forward ;). And the Java code should be more stable than the rest of the project. LGTM
|
+1 for propagating checked exceptions |
Adds Dependency.java to compiled files. Bug introduced in #41
A few minor changes.
Dependencyinto its own compilation unitFor Java code I prefer to propagate checked exceptions in the method types rather than wrapping them in
RuntimeExceptionor simply marking everything asthrows Exception. As code evolves this helps prevent inadvertent propagation of exceptions that can actually be handled; the programmer gets no indication due to the outer catch-all.