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

refactor: human-readable resource IDs use double underscore #2310

Merged
merged 10 commits into from
Feb 27, 2024

Conversation

yonadaa
Copy link
Contributor

@yonadaa yonadaa commented Feb 26, 2024

related to #2173

@yonadaa yonadaa requested review from alvrs and holic as code owners February 26, 2024 11:18
Copy link

changeset-bot bot commented Feb 26, 2024

🦋 Changeset detected

Latest commit: 122b309

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
@latticexyz/store-sync Patch
@latticexyz/dev-tools Patch
@latticexyz/common Patch
@latticexyz/cli Patch
@latticexyz/store-indexer Patch
@latticexyz/block-logs-stream Patch
@latticexyz/config Patch
@latticexyz/faucet Patch
@latticexyz/protocol-parser Patch
@latticexyz/store Patch
@latticexyz/world-modules Patch
@latticexyz/world Patch
@latticexyz/react Patch
@latticexyz/abi-ts Patch
create-mud Patch
@latticexyz/ecs-browser Patch
@latticexyz/gas-report Patch
@latticexyz/network Patch
@latticexyz/noise Patch
@latticexyz/phaserx Patch
@latticexyz/recs Patch
@latticexyz/schema-type Patch
@latticexyz/services Patch
@latticexyz/solecs Patch
solhint-config-mud Patch
solhint-plugin-mud Patch
@latticexyz/std-client Patch
@latticexyz/std-contracts Patch
@latticexyz/store-cache Patch
@latticexyz/utils Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

name,
}: {
readonly namespace: string;
readonly name: string;
Copy link
Member

@holic holic Feb 27, 2024

Choose a reason for hiding this comment

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

not blocking but this could also be Pick<Resource, 'namespace' | 'name'>

(we should also make Resource type use readonly props)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kicking this one until we fix Resource: #2321

@@ -0,0 +1,11 @@
export type ResourceLabel = `${string}__${string}`;

export function resourceLabel({
Copy link
Member

Choose a reason for hiding this comment

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

thoughts on getResourceLabel or resourceToLabel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely prefer one of them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going with resourceToLabel for consistency with resourceToHex

@@ -0,0 +1,11 @@
export type ResourceLabel = `${string}__${string}`;
Copy link
Member

@holic holic Feb 27, 2024

Choose a reason for hiding this comment

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

if you wanted to get real fancy, could make this generic, e.g.

export type ResourceLabel<namespace extends string = string, name extends string = string> = `${namespace}__${name}`;

only case I can think in which this might be useful is the RECS component metadata for a given table, where tableName would show up as e.g. store__Tables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh I like that! I was thinking namespace and name should kinda be types

holic
holic previously approved these changes Feb 27, 2024
Copy link
Member

@holic holic left a comment

Choose a reason for hiding this comment

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

just some tiny stylistic things, nothing particularly blocking

Co-authored-by: Kevin Ingersoll <[email protected]>
@yonadaa yonadaa merged commit d5c0682 into main Feb 27, 2024
11 checks passed
@yonadaa yonadaa deleted the yonadaaa/resource-label-underscores branch February 27, 2024 16:29
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.

2 participants