Cleaning up tests in scala3doc #10170
Conversation
|
@Kordyjan we should print the backticks I think, but I'm not sure if there's an obvious way to do that. We may want to open a separate issue for that. |
There was a problem hiding this comment.
LGTM
There is few nit-picks and I've asked for a few new features but this can be added in followup PR.
@pikinier20 could you take a look as well?
| def +:(other: Int): A | ||
| def :+(other: Int): A | ||
| // scala3doc has problems with names in backticks | ||
| // def `multi word name`: Int |
There was a problem hiding this comment.
We probably need a issue for it
There was a problem hiding this comment.
what are we missing here?
There was a problem hiding this comment.
Print a name that needs backticks in backticks, I believe.
This makes sense to include in Tasty-Reflect, since the general concept is being able to print a name in a form that corresponds to how it was entered in source. I'm not certain what's the best way to do that, though.
There was a problem hiding this comment.
We will need to introduce a Name abstraction in the reflection API. A String loses too much information about the name. We cannot distinguish semantic names from simple names and we cannot know if they were backticked. That is on my TODO list.
There was a problem hiding this comment.
Probably for now we can just fix that in scala3doc: lampepfl/scala3doc#235 (comment)
There was a problem hiding this comment.
@Kordyjan I'd rather avoid the code duplication, but I suppose that the regex for Scala identifiers that don't need a backtick is unlikely to change anytime soon. Can you wrap the code for detecting whether an identifier needs a backtick in a function whose name starts with hack, so we can grep for it easily? We have a bunch of functions like that in the doctool already.
There was a problem hiding this comment.
Actually, we won't have that info. The scaladoc will need to figure it out.
There was a problem hiding this comment.
@abgruszecki: #10199 is ready.
| ) | ||
|
|
||
| private def tastyFiles = | ||
| def collectFiles(dir: File): List[String] = dir.listFiles.toList.flatMap { |
There was a problem hiding this comment.
listFiles can return null if directory does not exisits so we should check that and print proper error message
| */ | ||
| enum Assertion: | ||
| case AfterPluginSetup(fn: DokkaContext => Unit) | ||
| case DuringValidation(fn: (() => Unit) => Unit) |
There was a problem hiding this comment.
We probably need to make this signature more explanatory. Probably does not need to match 1-to-1 to its Kotlin version
| case AfterDocumentablesTransformation(fn: DModule => Unit) | ||
| case AfterPagesGeneration(fn: RootPageNode => Unit) | ||
| case AfterPagesTransformation(fn: RootPageNode => Unit) | ||
| case AfterRendering(fn: (RootPageNode, DokkaContext) => Unit) |
There was a problem hiding this comment.
I think we should turn RootPageNode => Unit and (RootPageNode, DokkaContext) => Unit into traits and provide helper methods, e.g. option to inspect all pages or output of rendering or location of rendered page
| ) | ||
|
|
||
| extension [T] (s: T): | ||
| private def kUnit = kotlin.Unit.INSTANCE |
There was a problem hiding this comment.
Nice one, probably we can unified the kotlin.Unit handling in whole codebase after this PR is merged.
| .flatMap(signaturesFromSources(_, signatureKinds)) | ||
| .toList | ||
| val expectedFromSources: Map[String, List[String]] = allSignaturesFromSources | ||
| .collect { case Expected(name, signature) => name -> signature } |
There was a problem hiding this comment.
allSignaturesFromSources.collect { case e: Expected => e}.groupMap(_.name)(_.signature)
| findName(signature, signatureKinds).map(_ -> signature) | ||
| }.groupMap(_._1)(_._2) | ||
|
|
||
| val unexpected = unexpectedFromSources.flatMap(actualSignatures.getOrElse(_, Nil)) |
There was a problem hiding this comment.
unexpectedFromSources.flatMap(actualSignatures.get) should work here
…ghtly improved SignatureTests
The goals of this PR:
Problems and questions:
def `*name with backticks*`(param: Int): Intis documented asdef *name with backticks*(param: Int): Int. Is this the desired behavior?