-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Support doc task with dottydoc
#4952
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
This is only about supporting running |
| dependencyResolution.update(descriptor, updateConfiguration, warningConfiguration, log) match { | ||
| case Left(uw) => | ||
| throw new MessageOnlyException( | ||
| s"Couldn't retrieve `${scalaOrganization.value} %% $moduleName %% {scalaVersion.value}`.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing $ before {scalaVersion.value}?
| > run | ||
| > doc | ||
| > 'set initialCommands := "1 + 1" ' | ||
| # FIXME: does not work on the CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's preventing this from being run on the CI? It would be nice to also check that this generates what it's supposed to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The REPL doesn't work in the CI. JLine doesn't work outside of a "real terminal"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a way via some JLine option to support dumb or virtual terminals
(https://github.com/jline/jline3/wiki/Terminals) or sth. else... do we have an issue to investigate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default JLine spawns a "dumb" terminal if not able to create a terminal. But you can't do anything with it, not sure we can even use it for test purpose. So i disabled it:
https://github.com/lampepfl/dotty/blob/0d7826c8a77bfa0da727ac21213475739faa9428/compiler/src/dotty/tools/repl/JLineTerminal.scala#L21
project/Build.scala
Outdated
| compilerJar = jars.find(_.getName.startsWith("dotty-compiler_2.12")).get | ||
| } | ||
|
|
||
| // All compiler dependencies except the library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment needs to be updated?
| .map(_.data) | ||
| val otherDependencies = { | ||
| val excluded = Set("dotty-library", "dotty-compiler") | ||
| fullClasspath.in(`dotty-doc`, Compile).value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still interested in the compiler dependencies. Does this work because dotty-doc depends on dotty-compiler? Maybe add a comment
| @@ -1,4 +1,5 @@ | |||
| > run | |||
| > doc | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not run on PRs. Did you run it locally? I'll take your words for it 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine on my machine at least.
| } | ||
| }, | ||
|
|
||
| scalaInstance := Def.taskDyn { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a taskDyn? Is it to only evaluate fetchArtifactsOf if isDotty is true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
| import sbt.Keys._ | ||
| import sbt.librarymanagement.DependencyResolution | ||
| import sbt.internal.inc.ScalaInstance | ||
| // import sbt.inc.{ ClassfileManager, IncOptions } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this while you're at it
| ) | ||
| } | ||
|
|
||
| private def fetchArtifactsOf(moduleName: String) = Def.task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment:
/** Fetch artefacts for scalaOrganization.value %% moduleName % scalaVersion.value */17db5ac to
d52071f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please squash your commits
| val descriptor = dependencyResolution.wrapDependencyInModule(moduleID, scalaInfo) | ||
|
|
||
| dependencyResolution.update(descriptor, updateConfiguration, warningConfiguration, log) match { | ||
| case Left(uw) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: since uw is unused I would rewrite as:
... match {
case Right(report) =>
report.allFiles
case _ =>
throw new MessageOnlyException(...d52071f to
288d9b4
Compare
Fix #4965.