Skip to content
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

Fixes for RustWriter bugs #1465 & #1459 #1467

Merged
merged 1 commit into from
Jun 15, 2022
Merged

Fixes for RustWriter bugs #1465 & #1459 #1467

merged 1 commit into from
Jun 15, 2022

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented Jun 15, 2022

Motivation and Context

Description

Fix for two longstanding RustWriter bugs:

  1. docs didn't call ensureNewLine() which meant that when it was invoked after a call to writeInline the leading /// was never written
  2. formatting with :W didn't go through template rewriting which meant that it didn't work with case sensitivity.

This resolves both of those issues and adds unit tests.

Testing

  • unit test

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@rcoh rcoh requested a review from a team as a code owner June 15, 2022 15:34
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@rcoh rcoh enabled auto-merge (squash) June 15, 2022 15:59
@rcoh
Copy link
Collaborator Author

rcoh commented Jun 15, 2022

oh hmm...looking at the generated code diff, this adds extra newlines

@@ -259,6 +260,9 @@ fun <T : AbstractCodeWriter<T>> T.docs(text: String, vararg args: Any, newlinePr
// Rustdoc warns on tabs in documentation
it.trimStart().replace("\t", " ")
}
// Because writing docs relies on the newline prefix, ensure that there was a new line written
// before we write the docs
this.ensureNewline()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking into this from a completely wrong angle..

Copy link
Contributor

@crisidev crisidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this!

@rcoh rcoh merged commit b45c1f0 into main Jun 15, 2022
@rcoh rcoh deleted the writer-fixes branch June 15, 2022 16:20
@rcoh rcoh mentioned this pull request Jun 15, 2022
crisidev pushed a commit that referenced this pull request Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants