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

Minecraft Tags #7127

Open
wants to merge 18 commits into
base: dev/feature
Choose a base branch
from
Open

Conversation

sovdeeth
Copy link
Member

@sovdeeth sovdeeth commented Oct 3, 2024

Description

Adds support for minecraft's tag system.
Currently supports block, item, and entity type tags. Fluid and GameEvent are simple to add, but Fluid is useless and GameEvent is not in Skript just yet.

  • Support's Paper's material and entity tags. Paper tags look like paper:name_settag naturally. This converts them so the user sees and uses paper:name.
  • Supports the ability to get all tags of a item, block, or entity
  • Supports the ability to get all tags of a specific category (item/block/entity)
  • Supports the ability to get all the tags, period.
  • Allows filtering of tags by minecraft, datapack, paper, or custom (skript)
  • Namespaces are auto-populated if omitted, based on filtering settings:
    • No filtering, minecraft tags, and datapack tags all get the default "minecraft" namespace
    • Paper tags get "paper"
    • Custom tags get "skript"
    • This allows paper tag "wooden_doors" to resolve to paper:wooden_doors.
  • registering custom tags

Target Minecraft Versions: any
Requirements: none
Related Issues: #4924

@sovdeeth sovdeeth added the feature Pull request adding a new feature. label Oct 3, 2024
@sovdeeth sovdeeth marked this pull request as ready for review October 4, 2024 01:21
@sovdeeth sovdeeth requested a review from Fusezion October 4, 2024 01:22
Tests are kept deliberately small due to the volatile nature of tag contents.
Copy link
Member

@Efnilite Efnilite left a comment

Choose a reason for hiding this comment

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

quick glance

Comment on lines +57 to +62
return "tag \"" + tag.getKey() + "\"";
}

@Override
public String toVariableNameString(Tag<?> tag) {
return "tag: " + tag.getKey();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why can't the variable name and the string printed to string be the same output? Users will end up using a string for comparison by setting a variable to a string to be displayed in a way, and then use that same string for comparison.

Same issue was seen with itemtypes back in the day.
#4930
#1554
#913

Blueeyescat would find these alot https://github.com/SkriptLang/Skript/issues?q=is%3Aissue+author%3ABlueyescat

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't think having quotes in an index would be very nice. Do you think that's a concern?

src/main/java/org/skriptlang/skript/bukkit/tags/Tags.java Outdated Show resolved Hide resolved
src/main/java/org/skriptlang/skript/bukkit/tags/Tags.java Outdated Show resolved Hide resolved
* Gets all the tags of a specific origin that are of a specific type.
* @param origin The origin to filter by.
* @param type The type of tags to get.
* @return Tags from the given origin that are of the given type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* @return Tags from the given origin that are of the given type.
* @return Iterable<Tag<T>> tags from the given origin that are of the given type.

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the point of this? The return type is already part of the method signature, and this doesn't do anything special for javadocs.

@sovdeeth sovdeeth requested review from Moderocky and removed request for Fusezion October 13, 2024 18:38
Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

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

It looks ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants