Generate scaladoc for `@documentation` trait #731

zetashift · 2023-01-14T20:15:30Z

Attempt at resolving: #256
@kubukoz's feedback loop tips in #496 were very very helpful!
Still a draft, because I am not sure if I am following the logic of Renderer.scala correct everywhere.
However I was quite happy seeing this:

…trait

modules/codegen/src/smithy4s/codegen/internals/Renderer.scala

kubukoz

looking good so far, would love to see multi-line examples and a solution for escaping */. :)

modules/example/src/smithy4s/example/package.scala

modules/codegen/src/smithy4s/codegen/internals/Renderer.scala

zetashift · 2023-01-14T23:56:22Z

So another random question popped up.

Do we expect things like @constructor @tparam for certain members/shapes to also be generated, or does that fall out of scope?

kubukoz · 2023-01-15T01:52:17Z

I think that's not necessary at this stage... but @param might be easy to add in the future, and for methods corresponding to operations might be quite useful.

kubukoz · 2023-01-15T01:56:18Z

modules/codegen/src/smithy4s/codegen/internals/Renderer.scala

@@ -838,6 +856,7 @@ private[internals] class Renderer(compilationUnit: CompilationUnit) { self =>
    val closing = if (recursive) ")" else ""
    lines(
      deprecationAnnotation(hints),
+      documentationAnnotation(hints),
      obj(name, line"$Newtype_[$tpe]")(
        renderId(shapeId),
        renderHintsVal(hints),


(not related to this line in particular)

I think we're missing an example (and possibly an impl) for documenting structure members...

I guess this could look better if we do @param inside the structure's Scaladoc, as the lines would get quite lengthy if we just put it on the case class fields themselves. Maybe let's add this in a future PR.

In the meantime, I'd like to see an example of a documented alt case in a union as well. Ideally, every usage of documentationAnnotation(hints) would be checked with an example.

So should I make a DocumentationService.smithy file that has all these variants of docs on members/shapes/unions?

That works, but I meant that we need these cases to be test covered - not necessarily as part of a new spec, reusing existing ones is fine

zetashift · 2023-01-15T21:25:01Z

RE: multiline.

I got something working with :

  private def makeDocLines(l: List[String]): List[Line] = {
    def loop(l: List[String], acc: List[Line]): List[Line] = {
      l match {
        case Nil                  => acc
        case _ :: tl if tl == Nil => loop(tl, acc :+ line"  */")
        case hd :: tl             => loop(tl, acc :+ line"  * $hd")
      }
    }
    if (l.length > 1) loop(l.tail, List(line"/** ${l.head}")) else List(line"/** ${l.head} */")

  }
  private def documentationAnnotation(hints: List[Hint]): Lines = {
    hints
      .collectFirst { case h: Hint.Documentation => h }
      .foldMap { doc =>
        val lines = doc.docString.split(System.lineSeparator()).toList
        Lines(makeDocLines(lines))
      }
  }

However, as Baccata said, the number of lines/easyness could be better with a mutable solution, what fits better here? (or is my solution overthinking it?)

EDIT: renders the following:

/** This is the first line
  * Second line of information
  */
object AString extends Newtype[String] {

Baccata · 2023-01-17T09:13:41Z

@zetashift great work so far! I'm fine with the logic being implemented in terms of tailrec, it's fine.

Before we merge this in, I have a few asks :

I'd like for some multiline documentation to be added to some example specs (see the samples files and the examples project in sbt) so that we can have a look. The "example" project has checked-in code-generation results so that we can have a quick look at what happens when a PR changes the rendering logic.
I don't think @tparam or @constructor need to be generated, I do however think that @param should be generated for structures / operations (by aggregating the member documentations). So it'd probably be a good idea to change the Hint.Documentation to hold both the shape's docs and its fields when it has some.
Union members documentation should be generated above the Case-suffixed ADT members

zetashift · 2023-01-17T13:47:07Z

@Baccata sounds good!(I've been using the "example" project to check out my changes locally as well, thanks to kubukoz's tips).

As for point 2 and 3, I'll try to get that working.

kubukoz · 2023-01-17T14:58:20Z

Fields have their own hints, is it really necessary to change the parent's?

Baccata · 2023-01-17T15:04:58Z

Fields have their own hints, is it really necessary to change the parent's?

not 100% necessary, but it means def documentationAnnotation needs to be amended with a memberHints: List[List[Hint]] additional parameter and that's not looking too great

kubukoz · 2023-01-17T15:22:42Z

or we make an extra function that calls this after doing a local transformation. Only some callsites will actually need this

Baccata · 2023-01-17T15:26:20Z

Right. I think my point should be more that the purpose of the IR is to facilitate the rendering, by pre-digesting the raw data coming from the model. If the Renderer needs to apply logic to gather some data from several layers of the IR, the IR is not doing his job very well

kubukoz · 2023-01-17T16:40:17Z

Fair enough.

zetashift · 2023-01-18T16:05:19Z

modules/example/src/smithy4s/example/GetObjectInput.scala

+    ObjectKey.schema.required[GetObjectInput]("key", _.key).addHints(smithy.api.Required(), smithy.api.Documentation("Sent in the URI label named \"key\".\nKey can also be seen as the filename\nIt is always required for a GET operation"), smithy.api.HttpLabel()),
+    BucketName.schema.required[GetObjectInput]("bucketName", _.bucketName).addHints(smithy.api.Required(), smithy.api.Documentation("Sent in the URI label named \"bucketName\"."), smithy.api.HttpLabel()),


This doesn't seem to get rendered as a scaladoc

that's the aggregating the member documentations part, right? I don't see this having been implemented yet.

ah yes this should be rendered as @constructor arguments to GetObjectInput I believe, and I have not started yet with rendering those type of things

Yup, so the correct way to go at it would be for Hint.Documentation type to change to :

case class Documentation(docString: String, memberDocs: ListMap[String, String]) extends Hint

so that you'd be capturing the documentation of the members during the creation of the intermediate representation, and using it where it makes sense (ie, documentation on structures for now)

Thank you for the hint(pun not intended)! I understand that we want to capture the doc of the members (if there is any) in the IR. But how do I fill memberDocs in the SmithyToIR.scala file?

case doc: DocumentationTrait => Hint.Documentation(doc.getValue(), )

I've looked around a bit, and I cannot see where DocumentationTrait provides a function to get the member shapes. Do I have to make that logic myself?

that private def hints(shape: Shape) is kinda useless now. I only left it in because it has way too many usages... I guess you could remove it, rename shapeHints to hints and call it as this.hints wherever hints is taken.

Ah I see! I'll get to this weekend! :D, that clears a few things up.
After that I'll work on the escaping of docstrings.
And hopefully then I can undraft this PR.

sounds great. No pressure ;)

rename shapeHints to hints and call it as this.hints wherever hints is taken.

Do I rename it this.hints? The function hints isn't part of ShapeVisitor but SmithyIR
So (from other functions) this seems to work:

private def getDefault(shape: Shape): Option[Decl] = { val _hints = hints(shape)

if I'm understanding it right, and ShapeVisitor is nested in SmithyIR, you could still call it as SmithyIR.this.hints... but I'm kinda too lazy to try it out in this given I'd have to repeat that diff above 😂

just push what you have, we can iterate on that :)

modules/codegen/src/smithy4s/codegen/internals/Renderer.scala

kubukoz · 2023-01-18T22:33:04Z

modules/example/src/smithy4s/example/Foo.scala

+  /** this is a comment saying you should be careful for this case
+    * you never know what lies ahead with Strings like this
+    */


I'm cracking up 😂

modules/example/src/smithy4s/example/FooService.scala

modules/example/src/smithy4s/example/package.scala

zetashift · 2023-01-22T19:51:48Z

modules/codegen/src/smithy4s/codegen/internals/SmithyToIR.scala

+    case doc: DocumentationTrait =>
+      val memberDocs = shape
+        .members()
+        .asScala
+        .map(_.getTarget())
+        .map(shapeId => (shapeId.name, model.expectShape(shapeId)))
+        .map({ case (name, shape) =>
+          (name, shape.getTrait(classOf[DocumentationTrait]).asScala)
+        })
+        .collect({ case (name, Some(v)) => (name, v.getValue()) })
+        .toMap


memberDocs seems to always come up as an empty Map, and I don't know why exactly :S

I'll take a look

yeah so I think it's just that we were looking at the target type's docs, see https://github.com/disneystreaming/smithy4s/pull/731/files#r1083577898

I pushed a commit to fix that, also managed to squeeze in a couple cleanups (e.g. got rid of the tailrec function in favor of mkString): d3234c7

realized the linesIterator can be empty, so another one... 0b3bf43

kubukoz · 2023-01-23T00:42:06Z

modules/codegen/src/smithy4s/codegen/internals/SmithyToIR.scala

+        .members()
+        .asScala
+        .map(_.getTarget())
+        .map(shapeId => (shapeId.name, model.expectShape(shapeId)))


I think this shapeId.name should be the member name instead. Can't see what this outputs as, but I feel like that's going to be a different thing (e.g. in foo: String you'll see String instead of foo).

kubukoz · 2023-01-23T00:43:04Z

modules/codegen/src/smithy4s/codegen/internals/SmithyToIR.scala

+    case doc: DocumentationTrait =>
+      val memberDocs = shape
+        .members()
+        .asScala
+        .map(_.getTarget())
+        .map(shapeId => (shapeId.name, model.expectShape(shapeId)))
+        .map({ case (name, shape) =>
+          (name, shape.getTrait(classOf[DocumentationTrait]).asScala)
+        })
+        .collect({ case (name, Some(v)) => (name, v.getValue()) })
+        .toMap


I'll take a look

modules/codegen/src/smithy4s/codegen/internals/Renderer.scala

kubukoz · 2023-01-23T01:38:33Z

modules/codegen/src/smithy4s/codegen/internals/SmithyToIR.scala

@@ -738,19 +738,6 @@ private[codegen] class SmithyToIR(model: Model, namespace: String) {
      Hint.PackedInputs
    case d: DeprecatedTrait =>
      Hint.Deprecated(d.getMessage.asScala, d.getSince.asScala)
-    case doc: DocumentationTrait =>


Moved this out to a separate function to make it easier to get member docs even if the shape itself has no doc trait.

kubukoz · 2023-01-23T01:39:20Z

modules/codegen/src/smithy4s/codegen/internals/SmithyToIR.scala

+      .collect { case (name, Some(v)) => (name, v.getValue()) }
+      .toMap
+
+    // todo: what if the shape in question doesn't have docs, but members do?


I think we'll have to change the Hint.Documentation class to account for the shape not having docs. Just having two Options in it might be enough, or a Ior[ParentDocs, MemberDocs]

kubukoz · 2023-01-23T01:43:11Z

@zetashift I may've made a little mess in these commits, if it gets out of hand let me know and I can get this over the finish line. I guess at this point we'll need to:

make sure we have the things Olivier mentioned
add support for @param docs where the struct itself doesn't have its own docs (moved to Generate member Scaladoc for structs that don't have their own documentation #771)
support member docs on operation methods (moved to Generate method documentation for operations #772)

zetashift · 2023-01-23T02:21:17Z

@kubukoz I'll definitely try to get this over the finish line myself!

Two Options for Hint.Documentation sounds good to me, I'm not familiar with the Ior datatype.

…esent

zetashift · 2023-01-25T22:41:41Z

modules/codegen/src/smithy4s/codegen/internals/Renderer.scala

+      .flatMap { doc =>
+        val shapeLinesOpt = doc.docString.map(split(_)).map(_.toList)
+        val memberLinesOpt = doc.memberDocs.map(m => m.flatMap {
          case (memberName, memberDoc) =>
            split(memberDoc) match {
              case Some(NonEmptyList(head, rest)) =>
                s"@param $memberName $head" :: rest
              case None => Nil
            }


help, I'm losing the game of aligning types! :P

No but seriously, I'm having trouble getting the docs in a List as List(shapeLinesOpt, memberLinesOpt).flatten.flatten, as in I don't know how to get unnest these options(shapeDocs and memberDocs) using map or flatMap correctly.

pro-tip: you shouldn't have to : the lines() dsl method is there to flatten stuff for you, no matter the level of nesting. The code should roughly look like :

hints.collectFirst { .... }.foldMap { doc => lines( doc.docString.flatMap(split), doc.memberDocs.combineAll.foldMap { case (memberName, memberDoc) => //... } ) }

modules/codegen/src/smithy4s/codegen/internals/IR.scala

… friendly

zetashift · 2023-01-26T04:46:23Z

I'm trying to apply @Baccata suggestions of pre-splitting, and it was working out fine, but memberDocLines is a Map[String,List[String]] and but I'm still having trouble type checking it.

Ignoring the fact that my fp-fu is failing, I do think it might better to have Options(or something equivalent) in Hint.Documentation for indicating that shape docs or member docs might be absent

Baccata · 2023-01-26T23:16:12Z

PR'ing against your PR : https://github.com/zetashift/smithy4s/pull/1

Fix compilation and tweak rendering

zetashift · 2023-01-26T23:41:46Z

So I believe that only leaves this point:

Union members documentation should be generated above the Case-suffixed ADT members

Which looks good to me, anything I'm missing here :O?

Currently union shapes are rendered like this:

EDIT: nope nvm the StrCase doc is duplicated into the Foo shapes docs

kubukoz · 2023-01-27T00:40:17Z

modules/codegen/src/smithy4s/codegen/internals/Renderer.scala

@@ -701,8 +701,13 @@ private[internals] class Renderer(compilationUnit: CompilationUnit) { self =>
    val caseNamesAndIsUnit =
      caseNames.zip(alts.map(_.member == UnionMember.UnitCase))

+    val shapeDocs = hints


question: wait, how does this change actually help? I don't see anything union-related in the diff.

The Foo.scala union gets its @param str doc comment removed. It should be removed because the doc comment should be placed on case class StrCase

(sorry for the screenshot, I don't know how to do a fancy diff block :P)

I agree with the reasoning, but transformation is something that should happen in SmithyToIR, let's move it over there.

I've been pondering over how I would do that, without it getting a bit funky.
I think that would require that Documentation#shapeDocs would be a Hint[Documentation] rather than a List[String] no?

I actually can't figure out how to transform a Option[DocumentationTrait] into a Hint.Documentation where shapeDocs is a Hint.Documentation as well:

shape.getTrait(classOf[DocumentationTrait]).asScala.map { doc => val shapeDocs = Hint.Documentation(split(doc.getValue()), Map()) // Can't chuck in a List[String] in shapeDocs anymore Hint.Documentation(shapeDocs, memberDocs)

Do I just introduce a new type, like Hint.ShapeDocumentation?

I've given it some thoughts, and I think the most straightforward way is to keep the types as they are, but change this line to add a condition like if(shape.isUnion) Map.empty else ..., that'll be sufficient

So simple, love it! :P

Almost makes me think that it's too simple to work.

kubukoz · 2023-01-27T00:49:04Z

modules/example/src/smithy4s/example/FooService.scala

+  /** Returns a useful Foo
+    * No input necessary to find our Foo
+    * The path for this operation is "/foo"
+    */


question: would this also include parameter docs? I don't think we have an example of that

With parameter docs here you mean an input on an operation for example? I could chuck a doc comment on PutObjectin example.smithy

Let's defer this to a later PR, as there is a little bit of nuance to it with regards to packed inputs.

Baccata

Awesome work @zetashift, thank you so much for this contribution !

concern was addressed

zetashift · 2023-01-30T13:29:16Z

Thank you and @kubukoz very much for all the guidance and help! Some of this stuff I would never come up with on my own

Add Hints.Documentation to IR and attempt at rendering documentation …

14d3676

…trait

zetashift commented Jan 14, 2023

View reviewed changes

modules/codegen/src/smithy4s/codegen/internals/Renderer.scala Outdated Show resolved Hide resolved

kubukoz previously requested changes Jan 14, 2023

View reviewed changes

modules/example/src/smithy4s/example/package.scala Show resolved Hide resolved

modules/codegen/src/smithy4s/codegen/internals/Renderer.scala Show resolved Hide resolved

kubukoz reviewed Jan 15, 2023

View reviewed changes

Move doc annotation above deprecated annotation in Renderer

c6144d8

zetashift added 4 commits January 18, 2023 00:04

Add multiline rendering and add docstrings in example smithy files

da7fccc

Revert quoted string smithy file

59ee594

And also add the generated code for the quoted_string example

bacd07a

Fix triple docstring for GetFooOutput

576896a

zetashift commented Jan 18, 2023

View reviewed changes

Fix last doc line not rendering

52a6478

kubukoz reviewed Jan 18, 2023

View reviewed changes

zetashift added 4 commits January 20, 2023 02:19

makeDocLines gets in a NonEmptyList and minor typo fix

8a0926f

Add codegenerated code as well

c3d53ea

Inline loop for hd::Nil case

febe45f

Attempt at getting member docs to render

1d24ed8

zetashift commented Jan 22, 2023

View reviewed changes

zetashift added 3 commits January 22, 2023 21:47

Escape */ scaladoc characters using replace

482fba8

Fix typo in example doc

b2e8b8b

apply scalafmt to IR.scala

8c3504a

kubukoz reviewed Jan 23, 2023

View reviewed changes

Add member docs

d3234c7

kubukoz reviewed Jan 23, 2023

View reviewed changes

modules/codegen/src/smithy4s/codegen/internals/Renderer.scala Outdated Show resolved Hide resolved

Account for possible empty iterators

0b3bf43

kubukoz reviewed Jan 23, 2023

View reviewed changes

Attempt at getting member docs to render even if no shape docs are pr…

3c36b13

…esent

zetashift commented Jan 25, 2023

View reviewed changes

Baccata reviewed Jan 25, 2023

View reviewed changes

modules/codegen/src/smithy4s/codegen/internals/IR.scala Show resolved Hide resolved

Failed attempt at making Hint.Documentation more member doc rendering…

ca323cb

… friendly

Fix compilation and tweak rendering

e470236

Merge pull request #1 from disneystreaming/generate-scala-doc-baccata

57f869d

Fix compilation and tweak rendering

zetashift marked this pull request as ready for review January 26, 2023 23:41

Remove member docs for union shapes

97b6d4d

kubukoz reviewed Jan 27, 2023

View reviewed changes

Fix union rendering

a5b5c17

Baccata approved these changes Jan 30, 2023

View reviewed changes

Baccata merged commit 93122ca into disneystreaming:series/0.17 Jan 30, 2023

This was referenced Jan 30, 2023

Generate member Scaladoc for structs that don't have their own documentation #771

Closed

Generate method documentation for operations #772

Closed

Generate Scaladoc #256

Closed

		ObjectKey.schema.required[GetObjectInput]("key", _.key).addHints(smithy.api.Required(), smithy.api.Documentation("Sent in the URI label named \"key\".\nKey can also be seen as the filename\nIt is always required for a GET operation"), smithy.api.HttpLabel()),
		BucketName.schema.required[GetObjectInput]("bucketName", _.bucketName).addHints(smithy.api.Required(), smithy.api.Documentation("Sent in the URI label named \"bucketName\"."), smithy.api.HttpLabel()),

Generate scaladoc for @documentation trait #731

Generate scaladoc for @documentation trait #731

Conversation

zetashift commented Jan 14, 2023

kubukoz left a comment

Choose a reason for hiding this comment

zetashift commented Jan 14, 2023

kubukoz commented Jan 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetashift commented Jan 15, 2023 • edited Loading

Baccata commented Jan 17, 2023

zetashift commented Jan 17, 2023

kubukoz commented Jan 17, 2023 • edited by Baccata Loading

Baccata commented Jan 17, 2023

kubukoz commented Jan 17, 2023 • edited Loading

Baccata commented Jan 17, 2023 • edited Loading

kubukoz commented Jan 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetashift Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetashift Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubukoz commented Jan 23, 2023 • edited Loading

zetashift commented Jan 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetashift commented Jan 26, 2023

Baccata commented Jan 26, 2023

zetashift commented Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baccata left a comment • edited Loading

Choose a reason for hiding this comment

zetashift commented Jan 30, 2023

Generate scaladoc for `@documentation` trait #731

Generate scaladoc for `@documentation` trait #731

zetashift commented Jan 15, 2023 •

edited

Loading

kubukoz commented Jan 17, 2023 •

edited by Baccata

Loading

kubukoz commented Jan 17, 2023 •

edited

Loading

Baccata commented Jan 17, 2023 •

edited

Loading

zetashift Jan 20, 2023 •

edited

Loading

zetashift Jan 21, 2023 •

edited

Loading

kubukoz commented Jan 23, 2023 •

edited

Loading

zetashift commented Jan 26, 2023 •

edited

Loading

Baccata left a comment •

edited

Loading