Small improvements to Idea project generation #616
Conversation
|
|
||
| // The scala version here is non incidental | ||
| s"SBT: $groupId:$artifactId:$version:jar" | ||
| val artifactWithScalaVersion = artifactId.substring(artifactId.length - 5) match { |
There was a problem hiding this comment.
Can you comment what is and why it is happening here? Shouldn't the val be named artifactWithoutScalaVersion ?
There was a problem hiding this comment.
Forget about the suggested val name, I misread.
My concern is primary the explicit versions. What about 2.13?
There was a problem hiding this comment.
Yeah, I don't like it anymore than you do
The naming that Idea expects is very strange. I will add some examples/tests in order to clarify.
I ended up the writing the code that checks for the Scala version suffix that way because it mimics the way that the Idea Scala Plugin actually does it. I think it was a bad decision.
About appending "_2.12", Idea expects the version to match mill's version of Scala. I wanted to use the BuildInfo plugin for that, but I am not sure if mill itself is using it. I saw the version already hardcoded in the code, so I went with it. I will try to use the BuildInfo plugin instead.
|
I marked this PR as draft because I want it to have some kind of testing .and because I found myself adding more commits all the time (and because I wanted to test the github feature :D). Usually, I test manually with an example per issue, with mill itself, and with the projects my organization is using mill on. |
|
I modified the current |
|
@andres-pipicello-olx the intellij idea script doesn't really have that great coverage, so I honestly wouldn't mind "seems to work on my repos" as an acceptable test plan |
|
Sorry, I have no explicit pointers about testing this, besides GenIdeaTests, which you already mentioned. But if you want to test more complex cases, in principle, we could just call out to a sub process with complex enough mock project and check the expected outcome. But, as @lihaoyi already said, we can just merge now, if it works for you. But you have to change the draft status before we can merge. |
ghost
marked this pull request as ready for review
|
Ok, I marked the PR as ready |
|
Finally, I was able to convince Travis to build without errors (after lots of restarts, https://travis-ci.org/lefou/mill/builds/551179779), so I guess we're fine to merge. (We really need to fix the unreliable travis setup.) |
|
I'm going to keep each commit to keep the various bug fixes separated. |
|
Thanks for your contribution! |
Improves the resolution of
import $ivymagics