-
Notifications
You must be signed in to change notification settings - Fork 72
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
Remove rustfmt from progenitor-impl #368
Conversation
If this is what you had in mind, I'll update the README etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good direction; sorry it took me so long to get to this.
@@ -505,41 +505,19 @@ impl Generator { | |||
|
|||
/// Render text output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just delete generate_text and generatex_text_normalize_comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generate_text
has been in the README.md
sample build.rs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still! let's get rid of them!
@@ -26,6 +26,16 @@ where | |||
} | |||
} | |||
|
|||
fn generate_formatted(generator: &mut Generator, spec: &OpenAPI) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make the goal to have no changes in the output fixtures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still one changed fixture - progenitor-impl/tests/output/nexus-cli.out
- this is because the removed formatting code in cli.rs had different rustfmt settings, specifically it used only format_strings: Some(true),
which wasnt used in lib.rs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's it look like if we format_strings: Some(true)
for everything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it looks great - it does however change the non-cli .out files, so was optional which approach to take. I'll add it and you can judge for yourself, but IMO it is worth the extra changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good; one outstanding item for you to consider; one conflict to resolve and I'll merge it
ready to roll @ahl |
Thanks for doing this! |
Closes #362