Update renderer-blank line after imports, delete blank line from docs #1782

stefanadranca · 2024-01-29T18:19:07Z

Motivation:

We want to have a blank line between the imports and the generated code and no blank line between the docs and the protocols/methods declarations.

Modifications:

Updated the TextBasedRenderer and the tests.

Result:

The generated code will have the right format.

…m docs Motivation: We want to have a blank line between the imports and the generated code and no blank line between the docs and the protocols/methods declarations. Modifications: Updated the TextBasedRenderer and the tests. Result: The generated code will have the right format.

glbrntt · 2024-01-30T10:42:13Z

Sources/GRPCCodeGen/Internal/Renderer/TextBasedRenderer.swift

+      renderImport(`import`, isLast: isLast)
+      if isLast {
+        writer.nextLineAppendsToLastLine()
+        writer.writeLine("\n")


I don't think this is the right approach as it special cases the rendering of imports. I think adding a CodeBlockItem for an empty line is probably the better way to do this because it's more flexible: you can then use it in other places like between declarations, for example.

glbrntt · 2024-01-30T10:53:19Z

Sources/GRPCCodeGen/Internal/Renderer/TextBasedRenderer.swift

      commentString = string
    }
    if prefix.isEmpty {
+      while commentString.hasSuffix("\n") { commentString.removeLast() }


Where are the problematic extra new lines coming from with the preformatted comments? Modifying it here doesn't seem like the right thing to do, they are after all preformatted and we're re-formatting them...

I saw that they are stored in the "Location" objects that are created by protoc, so I wasn't sure where it would be best to do the change.

So if we have a proto that's like:

// Some comments about Foo service Foo { }

and get the comments using protoSourceComments(), do we get "// Some comments about Foo", "// Some comments about Foo\n" or "// Some comments about Foo\n\n" or something else?

It's not clear to me where the extra newline is coming from but I don't think we should reformat pre-formatted comments.

yes, we get "// Some comments about Foo\n"

Okay, so how come when we render it we get the extra new line? Does the writer add a newline?

The writeLine() function of the renderer that is used sets a property "nextWriteAppendsToLastLine" to false at the end, this way the new line to be rendered is on the next line not the current one (which is the blank one in our case)

I'm not sure that's the problem, I think the issue is that writeLine just appends a single String to an array of lines in the writer which are eventually joined by newlines. In other words the writer adds in new lines between each "line" added by writeLine.

However when we render the preformatted comments we append the whole block as a single line. The code below - for rendering the other comments - is different: it splits the lines and writes each separately. This lets the writer deal with adding new lines.

I think we should do the same but when the last line is empty (i.e. the text ended in "\n" before it was split) we should discard that empty line.

…ntation

glbrntt · 2024-01-30T16:16:06Z

Sources/GRPCCodeGen/Internal/Renderer/TextBasedRenderer.swift

+        } else {
+          // A blank line that should be dropped - pre formatted documentation
+          // might contain such lines.
+          return nil
+        }


I don't think this is right, for example I could give the following preformatted comment:

// Foo // Bar

and this would render as

// Foo // Bar

which isn't correct.

We only want to drop the line if it's empty and it has no prefix and it's the last line. We should also have tests for this.

glbrntt · 2024-01-30T16:18:08Z

Sources/GRPCCodeGen/Internal/StructuredSwiftRepresentation.swift

+  static func emptyLine() -> Self {
+    CodeBlock(item: .emptyLine)
+  }


This should be a static var:

Suggested change

static func emptyLine() -> Self {

CodeBlock(item: .emptyLine)

}

static var emptyLine: Self {

CodeBlock(item: .emptyLine)

}

glbrntt · 2024-01-30T16:19:51Z

Sources/GRPCCodeGen/Internal/Renderer/TextBasedRenderer.swift

  func renderComment(_ comment: Comment) {
    let prefix: String
-    let commentString: String
+    var commentString: String


Looks like this should still be a let

glbrntt · 2024-01-30T16:36:42Z

Sources/GRPCCodeGen/Internal/Renderer/TextBasedRenderer.swift

    switch description {
    case .declaration(let declaration): renderDeclaration(declaration)
    case .expression(let expression): renderExpression(expression)
+    case .emptyLine: writer.nextLineOnNewLine()


Can you add some tests for this?

glbrntt · 2024-01-30T16:39:14Z

Sources/GRPCCodeGen/Internal/Renderer/TextBasedRenderer.swift

+  /// Sets a flag on the writer so that the next call to `writeLine` starts on a new line.
+  ///
+  /// Safe to call repeatedly, it gets reset by `writeLine`.
+  func nextLineOnNewLine() { nextWriteAppendsToLastLine = false }


I'd probably bias towards modifying nextLineAppendsToLastLine as we reduce the number of functions:

func nextLineAppendsToLastLine(_ append: Bool = true) { ... }

glbrntt · 2024-01-30T16:40:40Z

Sources/GRPCCodeGen/Internal/Translator/TypealiasTranslator.swift


  func translate(from codeGenerationRequest: CodeGenerationRequest) throws -> [CodeBlock] {
-    var codeBlocks: [CodeBlock] = []
+    var codeBlocks: [CodeBlock] = [.emptyLine()]


I think this should be done by the caller instead. If you take this translator in isolation it's strange for it to return code blocks which start with an empty line.

glbrntt · 2024-01-31T09:53:18Z

Tests/GRPCCodeGenTests/Internal/Renderer/TextBasedRendererTests.swift

+      rendersAs: #"""
+
+        """#


I don't think the test tests what you think it tests 🙂

The last line of a multiline string literal doesn't include a line break. As the line is otherwise empty it means that the multiline string in your test is really just an empty string. (You should be able to replace it with "" and the test would be the same.)

From https://docs.swift.org/swift-book/documentation/the-swift-programming-language/stringsandcharacters#Multiline-String-Literals

A multiline string literal includes all of the lines between its opening and closing quotation marks. The string begins on the first line after the opening quotation marks (""") and ends on the line before the closing quotation marks, which means that neither of the strings below start or end with a line break:

let singleLineString = "These are the same." let multilineString = """ These are the same. """

FWIW the correct test here should be rendering as "\n"

glbrntt

I'm happy with this fix but a couple of small things need sorting first

glbrntt · 2024-01-31T12:59:41Z

Sources/GRPCCodeGen/Internal/Renderer/TextBasedRenderer.swift

-  /// the last stored line instead of starting a new line.
+  /// the last stored line or starts on a new line.
  ///
  /// Safe to call repeatedly, it gets reset by `writeLine`.
-  func nextLineAppendsToLastLine() { nextWriteAppendsToLastLine = true }
+  func nextLineAppendsToLastLine(_ append: Bool = true) { nextWriteAppendsToLastLine = append }


We can undo these changes since they're no longer necessary

glbrntt · 2024-01-31T13:03:18Z

...s/GRPCCodeGenTests/Internal/Translator/IDLToStructuredSwiftTranslatorSnippetBasedTests.swift

  func testSameGeneratedNameServicesSameNamespaceError() throws {
    let serviceA = ServiceDescriptor(
-      documentation: "Documentation for AService",
+      documentation: "/// Documentation for AService\n",


I don't think the service or method docs should be pre-formatted. We should only pre-format the leading trivia at the top of the proto file.

They are also extracted from the Location objects associated with the methods and services, so they are pre-formatted. I remember we've decided to have all the docs / comments from the proto file as pre-formatted and only the ones we generate as .docs

I thought we decided that only the copyright header would be pre-formatted because those are the only docs we should copy verbatim. I should've picked this up in the PR which added it.

The other docs are formatted by protoSourceComments() which inserts the leading ///, which isn't actually necessary as the renderer can (and should) do it instead.

Either way we can just leave it as is for now, it's not particularly important.

…grpc#1782) Motivation: We want to have a blank line between the imports and the generated code and no blank line between the docs and the protocols/methods declarations. Modifications: Updated the TextBasedRenderer and the tests. Result: The generated code will have the right format.

stefanadranca requested a review from glbrntt January 29, 2024 18:19

glbrntt requested changes Jan 30, 2024

View reviewed changes

stefanadranca and others added 3 commits January 30, 2024 10:54

Merge branch 'main' into sd-protobuf-documentation

9cd5b7f

CodeBlockItem for empty line + drop blank line in preformatted docume…

1f5c9b6

…ntation

small fixtures

884cd49

stefanadranca requested a review from glbrntt January 30, 2024 15:59

glbrntt requested changes Jan 30, 2024

View reviewed changes

implemented feedback

9e6ca51

stefanadranca requested a review from glbrntt January 30, 2024 17:34

glbrntt requested changes Jan 31, 2024

View reviewed changes

moved empty line logic to the renderer - changed how it is added

c2c36d4

stefanadranca requested a review from glbrntt January 31, 2024 11:26

glbrntt requested changes Jan 31, 2024

View reviewed changes

removed unnecessary change

403942e

stefanadranca requested a review from glbrntt January 31, 2024 13:40

glbrntt approved these changes Jan 31, 2024

View reviewed changes

stefanadranca merged commit 8ac3470 into grpc:main Jan 31, 2024

glbrntt added the version/v2 Relates to v2 label Feb 8, 2024

Update renderer-blank line after imports, delete blank line from docs #1782

Update renderer-blank line after imports, delete blank line from docs #1782

Uh oh!

Conversation

stefanadranca commented Jan 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanadranca Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanadranca Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glbrntt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stefanadranca Jan 30, 2024 •

edited

Loading

stefanadranca Jan 30, 2024 •

edited

Loading