chore(rust): cargo fmt and refactor into two workspace with two crates #164
Conversation
|
Thanks for this @willemneal! Does it always use spaces for formatting? It's odd when it introduces spaces when the rest of the repo is using tabs. I'm not personally a fan of how the formatter likes to put function parameters on new lines like that, but I'm not against it if that's the recommended way :P |
|
Looks like some conflicts have come up as well |
|
Switched to use tabs and since you had added the proc_macro, I moved the core into its own folder. This makes it clear what the top level exports are. I also added a lop level |
api/rust/crates/core/Cargo.toml
Outdated
| @@ -1,5 +1,5 @@ | |||
| [package] | |||
| name = "suborbital" | |||
| name = "suborbital-core" | |||
There was a problem hiding this comment.
Hmm, I don't think we can change this, since then the package name on Cargo would change, no?
There was a problem hiding this comment.
I thought this would work since the parent crate still had the name. However, after some digging it turns out you can't publish a crate with local dependencies.
See: rust-lang/cargo#1565 (comment)
So I reverted to using two crates, that will need to be published separately.
There was a problem hiding this comment.
If you refactor that anyway ... I always wanted to ask why there is additional folder depth in the rust directory?
We have:
- .../api/assemblyscript/code
- .../api/rust/suborbital/code
- .../api/swift/code
- .../api/tinygo/code
Would it be possible to remove the extra dir to make it more coherent.
It should not make a difference regarding the crate. The crates name is defined in the Cargo.toml.
There was a problem hiding this comment.
Yeah, we can get rid of the extra depth, I did that when I didn't fully understand how Rust crates were organized :)
Also, I don't think I want to have a fully separate crate for the macro? Is there any reason it can't just be a module inside the existing crate?
There was a problem hiding this comment.
Derive Macros have to be defined in their own crate as described in the docs:
Procedural macros must be defined in a crate with the crate type of proc-macro.
That seems to be how the crate system works in that regard.
There was a problem hiding this comment.
Unfortunately a proc_macro must be inside its own crate. And as stated above you have to publish local dependencies if you want to use them in your published your crate. So both need to be published. I've renamed the macro to a non-generic name to prepare for this.
So in this case it's unavoidable to have two crates. However, Rust projects are often broken up into crates. In fact, I would suggest a further refactoring to pull out all extern definitions to their own crate like is done here: https://github.com/near/near-sdk-rs/blob/master/sys/src/lib.rs
Personally I find it very useful for a Wasm project with non-standard imports to collect them all in one place. Then have the associated wrappers depend on that.
The other win for using workspaces is that you can use private crates. This lets you provide an example and test together.
Lastly, separating your API and types from your implementation is another benefit, although, in this case it's definitely overkill.
There was a problem hiding this comment.
An own crate for the extern stuff sounds, I like the separation that creates.
|
Other than the last open thread/question, this looks great! I'll approve once we have that answered. |
Add top level cargo toml since it helps with IDE support.
willemneal
force-pushed
the
chore/rust_fmt
branch
from
d1d5af8 to
e101bef
Compare
willemneal
changed the title
Like I said I'm happy with a bigger value, but I'm also just another contributor. From my side feel free to increase the value 👍 |
|
@cohix It now includes the option runnable PR's updates. |
|
Also moved re-export to make it clearer. |
This updates the files with the standard rust formatter. If this isn't welcomed, I understand.
If it is welcomed, would you like to add a CI check or a git hook?