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

Add tool to format with pretty-please #2739

Closed
wants to merge 4 commits into from
Closed

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Jun 1, 2023

Motivation and Context

This PR adds a tool that formats a cargo workspace using the dtolnay/prettyplease library to replace rustfmt since rustfmt fails to format most of our generated code now.


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

@github-actions
Copy link

github-actions bot commented Jun 1, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti marked this pull request as ready for review June 1, 2023 16:14
@jdisanti jdisanti requested review from a team as code owners June 1, 2023 16:14
@github-actions
Copy link

github-actions bot commented Jun 1, 2023

@jdisanti
Copy link
Collaborator Author

jdisanti commented Jun 1, 2023

Looks like prettyplease has a bug that the SDK triggers:

    Checking aws-sdk-transcribestreaming v0.0.0-local (/home/build/workspace/smithy-rs/aws/sdk/build/aws-sdk/sdk/transcribestreaming)
error: unnecessary trailing semicolon
  --> /home/build/workspace/smithy-rs/aws/sdk/build/aws-sdk/sdk/transcribestreaming/src/event_stream_serde.rs:17:9
   |
17 |         ; Ok(::aws_smithy_eventstream::frame::Message::new_from_parts(headers, payload))
   |         ^ help: remove this semicolon
   |
   = note: `-D redundant-semicolons` implied by `-D warnings`

Edit: Nevermind. I was quick to jump to conclusions. That's our code generator's fault. The new tool failed to run during SDK codegen, and the rustfmt had been removing this redundant semicolon.

@jdisanti
Copy link
Collaborator Author

jdisanti commented Jun 1, 2023

I removed the cargo metadata workspace resolution from the tool since that was what was causing it to fail for the SDK (since it runs before the SDK copies over the runtime crates, and thus, the runtime crates don't exist when resolving the Cargo.toml files with cargo metadata). Now it just recursively formats all Rust files in a given directory.

@github-actions
Copy link

github-actions bot commented Jun 1, 2023

@jdisanti
Copy link
Collaborator Author

jdisanti commented Jun 1, 2023

Judging from the codegen diff, prettyplease removes all non-doc comments. Not sure if this will work for us.

Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

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

Closes #1526.

This would be nice but we need it to preserve comments :(

fileManifest.baseDir,
timeout = settings.codegenConfig.formatTimeoutSeconds.toLong(),
)
} catch (err: CommandFailed) {
logger.info(
"[rust-server-codegen] Failed to run cargo fmt: [${service.id}]\n${err.output}",
"Failed to run please-fmt (be sure to install the tool with " +
"`cargo install --path tools/ci-build/please-fmt`): [${service.id}]\n${err.output}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"`cargo install --path tools/ci-build/please-fmt`): [${service.id}]\n${err.output}",
"`cargo install --locked --path tools/ci-build/please-fmt`): [${service.id}]\n${err.output}",

[workspace]

[dependencies]
argh = "0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of this over clap?

@jdisanti
Copy link
Collaborator Author

Prettyplease doesn't preserve comments and the maintainer doesn't want to support that, so it's not a viable option for smithy-rs.

@jdisanti jdisanti closed this Jun 20, 2023
@jdisanti jdisanti deleted the jdisanti-pretty-please branch November 30, 2023 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants