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

consolidate move struct and resource type #120

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

0xmigo
Copy link
Contributor

@0xmigo 0xmigo commented Oct 25, 2023

Description

resolve #117

Removing the MoveStructType and consolidate into MoveResourceType.

We could keep both, and assign one to another. i.e

export type MoveStructType = MoveResourceType;
export type MoveResourceType = `${string}::${string}::${string}`;

But I feel like it would just confuse the user.

Test Plan

pnpm fmt

Related Links

@xbtmatt
Copy link
Contributor

xbtmatt commented Oct 25, 2023

I think MoveStructType makes more sense if we have to use one (because function names aren't MoveResources technically), but honestly the type alias solution might be the most accurate. Maybe there's a more formal name for the 1::2::3 type format in the Move book that encompasses function names and structs/resources

@0xmigo
Copy link
Contributor Author

0xmigo commented Oct 25, 2023

@xbtmatt Yeah I was thinking about using a alias option that I mentioned in the description. Not sure if that will confuse the user though.

@0xmaayan
Copy link
Collaborator

@xbtmatt Yeah I was thinking about using a alias option that I mentioned in the description. Not sure if that will confuse the user though.

Let's not use alias, that only creates confusion. We should go with a type name that makes sense to us

@@ -667,7 +667,6 @@ export type MoveUint128Type = string;
export type MoveUint256Type = string;
export type MoveAddressType = string;
export type MoveObjectType = string;
export type MoveStructType = `${string}::${string}::${string}`;
export type MoveOptionType = MoveType | null | undefined;
/**
* String representation of a on-chain Move struct type.
Copy link
Contributor Author

@0xmigo 0xmigo Oct 25, 2023

Choose a reason for hiding this comment

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

@xbtmatt Description will live here. Updating to name MoveStructType

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should say something like...

/**
* This is the format for a fully qualified struct, resource, or entry function in Move.
*/

Above it or something

@0xmigo 0xmigo force-pushed the jin/consolidate_MoveResourceType branch from 2ba5f67 to 1562934 Compare October 26, 2023 17:07
@0xmigo 0xmigo merged commit ad8aa16 into main Oct 26, 2023
4 checks passed
@0xmigo 0xmigo deleted the jin/consolidate_MoveResourceType branch October 26, 2023 17:48
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.

[consistency] Document and reduce number of duplicated types
3 participants