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

Tidy subxt-codegen crate interface #1225

Merged
merged 7 commits into from
Oct 27, 2023
Merged

Tidy subxt-codegen crate interface #1225

merged 7 commits into from
Oct 27, 2023

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Oct 25, 2023

This crate interface has always been a bit all over the place, being intended primarily for internal use. However, that's not been clear, and so it has been used.

This PR:

  • Hides or removes most of the codegen interface, exposing only one builder struct which allows for code to be generated using the same options provided via the #[subxt] macro or subxt codegen CLI tool. The aim here is to provided a supported third means to generate the interface; via a rust library instead of macro/CLI.
  • Exposes a few things in a hidden __private module that should not be relied on, just because subxt-explorer is currently relying on them.
  • Makes the fetch-metadata flag very much opt-in (the codegen stuff should just be pure code really; we only have these fns here because they would need to live in a new crate otherwise).
  • Removes CratePath and some other bits that were sortof pointless (and I didn't want to expose such a thing in the public interface).

With this, I'd be happy saying that we supported people using the codegen crate directly. Ultmately, the type generation code should all be moved into a separate crate and the overall interfaces tidied up etc. This PR will allow that to happen without affecting the subxt-codegen crate (hopefully).

@jsdw jsdw requested a review from a team as a code owner October 25, 2023 10:39
Copy link
Contributor

@tadeohepperle tadeohepperle left a comment

Choose a reason for hiding this comment

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

Great PR, looks good to me, I just had a few small comments/questions :)

.map_err(|e| eyre!("Cannot decode the provided metadata: {e}"))?;
let code = codegen
.generate(metadata)
.map_err(|e| eyre!("Cannot generate code: {e}"))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is so much cleaner now 👍

codegen/src/fetch_metadata.rs Outdated Show resolved Hide resolved
codegen/src/fetch_metadata.rs Outdated Show resolved Hide resolved
#[doc(hidden)]
pub mod __private {
pub use crate::types::{DerivesRegistry, TypeDefGen, TypeGenerator, TypeSubstitutes};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for keeping it exposed, later we can have a cleaner solution.


/// A newtype wrapper which stores the path to the Subxt crate.
#[derive(Debug, Clone)]
pub struct CratePath(syn::Path);
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that this got removed, it was a bit annoying to work with.

codegen.set_target_module(item_mod);

// Use the provided crate path:
if let Some(crate_path) = args.crate_path {
Copy link
Contributor

Choose a reason for hiding this comment

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

The args used in the macro are very similar to the args used in the CLI, but I think it would be difficult/not worth it to unify them somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah; I mean ideally they would be about identical. The types will vary a bit just because they come from different sources (eg strings from a terminal vs syn::Paths from a lib call or whatever)

.generate_docs(should_gen_docs)
.runtime_types_only(args.runtime_types_only);
// Do we want to fetch unstable metadata? This only works if fetching from a URL.
let unstable_metadata = args.unstable_metadata.is_present();
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reasoning behind allowing unstable metadata only from URLs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh; this is just because if you provide the metadata, you can't pick what version it will be, but if you fetch from a URL you can decide what to fetch. No real need for this option at the mo since it wouldn't actually work, but when we start work on V16 metadata at some point it'll become useful again to try it out in Subxt :)

)
let mut codegen = CodegenBuilder::new();
codegen.set_target_module(item_mod);
let generated_code = codegen
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks way better! Nice!

Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! This API improves a lot the user experience for our codgen crate!

@jsdw jsdw merged commit 2b21f8d into master Oct 27, 2023
10 checks passed
@jsdw jsdw deleted the jsdw-tidy-codegen branch October 27, 2023 15:35
@jsdw jsdw mentioned this pull request Dec 7, 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.

None yet

3 participants