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 cargo-typify: CLI for generating Rust Types #204

Merged
merged 33 commits into from
Mar 16, 2023

Conversation

coreyja
Copy link
Contributor

@coreyja coreyja commented Feb 26, 2023

Adds a CLI called cargo-typify to the Workspace to be able to generate the Rust types

This is made to work as a cargo sub-command, and was tested with cargo install --path . locally, and then I was able to use cargo typify input.json as suggested

By default we take the input.json file and write an input.rs file, where we change the extension to rs.

You can use the --output some_path.rs to choose a specific path
Or use --output - to output to stdout

You can also use --builder to opt into the Builder Interface

And --additional-derive to add a new Trait to be derived for all structs

Fixes #175

This creates a new binary package to use and adds it to the workspace.

Unfortunately there doesn't seem like a good way to make it the default
run command for the entire workspace.
See this issue for more details: rust-lang/cargo#7290
This is a really bare bones version, but its working

We use `clap` for Arg parsing, and `color_eyre` for error handling.

We take an `input` arg and an optional `output` arg that both take a path to a file.
If no output is specified, we print to stdout.
Cargo.toml Outdated Show resolved Hide resolved
let base_type = &schema.schema;

// Only convert the top-level type if it has a name
if let Some(base_title) = &(|| base_type.metadata.as_ref()?.title.as_ref())() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stole this from the build.rs example, mostly commenting on it cause I like the closure to be able to use ? on the Option!

Haven't seen that pattern before so TIL 💯

@coreyja coreyja changed the title Add cargo-typify a CLI for generating the Rust Types Add cargo-typify: CLI for generating the Rust Types Feb 26, 2023
@coreyja coreyja changed the title Add cargo-typify: CLI for generating the Rust Types Add cargo-typify: CLI for generating Rust Types Feb 26, 2023
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

Looking great!

I'd like to see some testing. My suggestion is to "hollow out" main to do most of the work in a function that does arg parsing and returns a String of all the code. Use expectorate to validate that the output is what we expect.

I'd suggest using example.json or something and then run with various options.

It might also be good to have a system test that actually invokes the command; let me know if you need help with that.

nit: I think you'll want to copy typify-impl/release.toml to this new sub-crate for when I publish it.

Have you tried pointing cargo install at this? My hope is that people could cargo typify <schema.json> as a part of their build process.

cargo-typify/src/main.rs Outdated Show resolved Hide resolved
cargo-typify/src/main.rs Outdated Show resolved Hide resolved
cargo-typify/src/main.rs Outdated Show resolved Hide resolved
cargo-typify/Cargo.toml Show resolved Hide resolved
@ahl
Copy link
Collaborator

ahl commented Mar 8, 2023

This was a great start; I'd encourage you to continue!

* main:
  fix spelling (oxidecomputer#214)
  make dependency on typify-macro optional (oxidecomputer#213)
  Bump trybuild from 1.0.77 to 1.0.79 (oxidecomputer#209)
  Bump unicode-ident from 1.0.6 to 1.0.8 (oxidecomputer#208)
  Bump serde_json from 1.0.93 to 1.0.94 (oxidecomputer#210)
  Bump paste from 1.0.11 to 1.0.12 (oxidecomputer#211)
  Bump thiserror from 1.0.38 to 1.0.39 (oxidecomputer#212)
  Bump schemars from 0.8.11 to 0.8.12 (oxidecomputer#205)
  Bump syn from 1.0.107 to 1.0.109 (oxidecomputer#206)
@coreyja
Copy link
Contributor Author

coreyja commented Mar 11, 2023

Ok! Took another pass at this, sorry it took a bit for me to come back to it

I addressed most of the comments here!

Changes

I broke out lib.rs and wrote tests in there
I then also created an integration test file, and kinda liked that level better. So most of the testing is done at the integration level right now
Happy to go either direction here, but kinda thinking I will delete the tests in lib.rs in favor of the integration tests.

We now also support additional derives and opting into the builder interface

Todo

There are some things remaining that I want to look at tomorrow

  • Test locally with Cargo to make sure cargo typify some_file.json works as expected
  • Look into type_mod more. I didn't see a different in the generated Rust with or without it but need to do more digging

@ahl
Copy link
Collaborator

ahl commented Mar 13, 2023

This is looking good. What do you think about a test shows the usage/help output?

Can you resolve the Cargo.lock conflicts?

@coreyja
Copy link
Contributor Author

coreyja commented Mar 13, 2023

This is looking good. What do you think about a test shows the usage/help output?

Yes, I like that!

Can you resolve the Cargo.lock conflicts?

Yup!

Will push a change this afternoon/evening

…on by default. You can opt into either a file or stdout
# By dependabot[bot] (5) and John Vandenberg (1)
# Via GitHub
* main:
  expand docs for TypeSpaceSettings (oxidecomputer#202)
  Bump serde from 1.0.152 to 1.0.155 (oxidecomputer#217)
  Bump proc-macro2 from 1.0.51 to 1.0.52 (oxidecomputer#219)
  Bump quote from 1.0.23 to 1.0.24 (oxidecomputer#220)
  Bump serde_tokenstream from 0.1.6 to 0.1.7 (oxidecomputer#218)
  Bump regress from 0.4.1 to 0.5.0 (oxidecomputer#216)

# Conflicts:
#	Cargo.lock
Comment on lines +6 to +11
#[derive(Parser)] // requires `derive` feature
#[command(name = "cargo")]
#[command(bin_name = "cargo")]
enum CargoCli {
Typify(CliArgs),
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added to make the cargo sub-command work nicely!

To run the cli locally you now need to do something like the following:

cargo run -- typify some_file.json

This style was taken from the Clap cookbook: https://docs.rs/clap/latest/clap/_derive/_cookbook/cargo_example_derive/index.html

@coreyja
Copy link
Contributor Author

coreyja commented Mar 13, 2023

Ok!

I think this is ready to go now! Edited the PR description to match the end PR

Changes since last time:

  • We now default to making a new file next to the input, with an rs extension. I thought this was the nicest default for a user. I added --output - to use stdout
  • I removed the type_mod setting since it didn't have an effect on the output (at least for the example.json file). I can add that back in if I mis-understood!
  • Changes to the CLI arg parsing to support being run as a cargo sub-command

@coreyja coreyja marked this pull request as ready for review March 13, 2023 22:03
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

one change and a windows failure (possibly due to newlines?)


/// Whether to include a builder-style interface
#[arg(short, long, default_value = "false")]
pub builder: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry I didn't think of this earlier. I'd like to make the builder style the default. What do you think about two flags: --builder and --positional that are mutually exclusive?

@coreyja
Copy link
Contributor Author

coreyja commented Mar 15, 2023

Ok tests should pass on this latest commit!

Used the same library as expectorate was using to covert line endings

Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

This looks great. I'm going to file a few follow-on issues. Thanks for doing this.

@ahl ahl merged commit b0bdbff into oxidecomputer:main Mar 16, 2023
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.

command line tool to output rust code [HELP WANTED]
2 participants