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

Allow for exclamation marks in resource locations. #1539

Closed
wants to merge 4 commits into from
Closed

Allow for exclamation marks in resource locations. #1539

wants to merge 4 commits into from

Conversation

NateKootstra
Copy link

Replacing the initialization of LegalResourceLocationCharacters in packages/core/src/parser/resourceLocation.ts with the following should solve issue #1496:

export const LegalResourceLocationCharacters = new Set([ 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '_', '-', '.', '!', ])

This should allow for exclamation marks to be used to remove components from items without Spyglass listing it as an error, which would be in line with how it works in game.

In theory this will allow examples such as "!minecraft:food" to function, although "!food" would likely still result in a warning due to Spyglass interpreting it as "minecraft:!food". If anyone more experienced with Spyglass knows to fix that it would definitely improve the solution. I'll spend some more time trying to figure it out myself.

@jacobsjo
Copy link
Contributor

This is almost certainly the wrong place to fix this.

There are other places that use resource locations. These places do not allow exclamation marks. The exclamation mark is part of the syntax to remove components from items, its not part of the resource location itself.

@NateKootstra
Copy link
Author

NateKootstra commented Jul 20, 2024

That's fair. Do you think that modifying packages/core/src/common/util.ts with something like this could work?

export function lengthen(value: string): FullResourceLocation {
	switch (value.indexOf(NamespacePathSep)) {
		case -1:
			return value.startsWith('!') ? `!${DefaultNamespace}${NamespacePathSep}${value.replace('!', '')}` : `!${DefaultNamespace}${NamespacePathSep}${value}`
		case 0:
			return (value.startsWith('!') ? `!${DefaultNamespace}${value.replace('!', '')}` : `${DefaultNamespace}${value}`) as unknown as FullResourceLocation
		default:
			return value as FullResourceLocation
	}
}

If not, what would the proper level to fix this at be?

@NateKootstra
Copy link
Author

Obviously after that we still need to allow for the exclamation mark in the FullResourceLocation.

@NeunEinser
Copy link
Contributor

NeunEinser commented Jul 20, 2024

This should be fixed in the item argument type specifically.

You can take a look at https://github.com/SpyglassMC/Spyglass/blob/main/packages/mcdoc/src/runtime/attribute/builtin.ts which includes a parser for resource locations in JSON / SNBT, which already works for !-prefixed item components in e.g. the set_components loot function.

Simply allowing an exclamation mark inside of resource locations is an inadequate solution for following reasons:

  • item components are a registry, and you need to avoid the "undeclared symbol" warning when using a prefixed resource location. The exclamation prefixes themselves are not part of the registry and thus would not be known by Spyglass.
  • auto complete should correctly suggest prefixed resource locations.

Something similar may also be implemented for the type selector argument, though I am not very familiar with that part of the code.

@NateKootstra
Copy link
Author

Thanks a ton for the help! I'll look into that when I get home.

@NateKootstra
Copy link
Author

Yeah to be honest I'm incredibly lost when it comes to navigating through this project. I'll give it another shot tomorrow. Where Spyglass actually start analyzing mcfunction components?

@NateKootstra NateKootstra closed this by deleting the head repository Jul 27, 2024
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