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

ty: Make type fields public #176

Merged
merged 7 commits into from
Mar 29, 2023
Merged

ty: Make type fields public #176

merged 7 commits into from
Mar 29, 2023

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Mar 28, 2023

Subxt needs access to the types stored in the PortableRegistry to create a slim subset of the metadata,
where the generic types of a Type are ignored.

To achieve this, subxt will access something similar to: registry.types.get(index).ty.type_param.

While at it, make the other fields public. The old getters are still left in place for backward compatibility.

cc @paritytech/subxt-team

lexnv added 2 commits March 28, 2023 19:41
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv self-assigned this Mar 28, 2023
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@jsdw
Copy link
Contributor

jsdw commented Mar 29, 2023

I could get behind making fields public and treating them as just bundles of data; a description of the shape of a type.

I wonder whether there are any forseeable reasons to keep this internal structure hidden; @ascjones perhaps you have thoughts?

If we do go this route I'd also be tempted to add a #[deprecated(...)] attr to each of the getter functions to encourage people over to just accessing the fields directly (it would be nice ultimately to be consistent and remove the getters in the next major version).

The attr could look something like:

#[deprecated(since = "2.5.0", note = "prefer to access the fields directly; this getter will be removed in the next major version")]

That said I'd have to test where warnings will show for this, and wouldn't want warnings appearing for anybody who is only indirectly using scale-info at the latest version via some other crate (since they couldn't do anything about it).

The other option is to do a major version bump and remove the getters immediately, but that would create more churn..

@lexnv
Copy link
Contributor Author

lexnv commented Mar 29, 2023

The other option is to do a major version bump and remove the getters immediately, but that would create more churn

Yep indeed. Either way we pick seems solid and we'll eventually end up removing them (waiting for @ascjones to confirm this).

That being said, we could remove them now and handle the breaking changes individually:

  • create a new release with scale-info
  • update + release every crate that depends on this in the substrate dependency graph (changes are trivial to modify, although a bit of effort to track everything down)

@ascjones
Copy link
Contributor

I'm fine with making these public.

Regarding the getters, probably best to deprecate them for the least disruption, then remove for a 3.0 release.

lexnv added 2 commits March 29, 2023 13:45
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@jsdw
Copy link
Contributor

jsdw commented Mar 29, 2023

This looks good to me!

I think that #[deprecated] warnings in sub-dependencies won't trickle up (eg if A depends on B which depends on C, and C uses #[deprecated] on some stuff B is using, then if I compile A I would hope not to see those warnings). I bring this up just because it could be quite noise for anybody transitively depending on scale-info anywhere if this isn't actually the case. Would be good to confirm if possible :)

@jsdw
Copy link
Contributor

jsdw commented Mar 29, 2023

I had a go locally and created 3 crates in 3 folders seen below. When I runcargo check in temp I see the deprecated warning. Maybe this is different for published crates, or maybe deprecated warnings do bubble up through all crates?

temp:

cargo.toml:

[dependencies]
temp2 = { path = "../temp2" }

lib.rs

pub fn add(left: usize, right: usize) -> usize {
    temp2::add(left, right)
}

temp2

cargo.toml:

[dependencies]
temp3 = { path = "../temp3" }

lib.rs

pub fn add(left: usize, right: usize) -> usize {
    temp3::add(left, right)
}

temp3

lib.rs

#[deprecated]
pub fn add(left: usize, right: usize) -> usize {
    left + right
}

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Copy link
Contributor Author

lexnv commented Mar 29, 2023

I've had a go at this in paritytech/subxt#879.

What I did there was to point scale-info to this branch in Cargo.toml

[patch.crates-io]
scale-info = { git = "https://github.com/paritytech/scale-info.git", branch = "lexnv/resolve_mut" }

Warnings did appear for all subxt usages across the repo, but after fixing those individually in this commit they went away.

@jsdw
Copy link
Contributor

jsdw commented Mar 29, 2023

I asked in the reddit and warnings apparently only show up for local crates, not those published to crates.io, so I think all is good :)

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv merged commit 78ff15a into master Mar 29, 2023
@lexnv lexnv deleted the lexnv/resolve_mut branch March 29, 2023 16:58
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.

3 participants