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

Add Registry#getTags #11795

Merged
merged 5 commits into from
Dec 26, 2024
Merged

Conversation

Machine-Maker
Copy link
Member

Closes #11758

@Machine-Maker Machine-Maker requested a review from a team as a code owner December 23, 2024 21:14
@kennytv kennytv added the type: feature Request for a new Feature. label Dec 24, 2024
@ShaneBeee
Copy link

First of all, thank you for doing this, I appreciate it.

Secondly, I didnt really properly think about this when I asked for getTags, but is it possible to also add getTagKeys to return a list of TagKeys?

@Machine-Maker
Copy link
Member Author

Machine-Maker commented Dec 25, 2024

You can pretty easily just map the Stream from Stream<Tag<T>> getTags right? That's one of the reasons a Stream here makes sense, lots of filtering or mapping or flatmapping people might want to do, so why turn it into a list just to go back into a stream again.

@ShaneBeee
Copy link

ShaneBeee commented Dec 25, 2024

You can pretty easily just map the Stream from Stream<Tag<T>> getTags right? That's one of the reasons a Stream here makes sense, lots of filtering or mapping or flatmapping people might want to do, so why turn it into a list just to go back into a stream again.

Sorry I may have miss worded that. Forget I said "list".
The point I was trying to make was something to return TagKeys instead of just Tags since Paper seems to be more keen to using TagKey and other things along those lines, felt like it fits more.

Obviously I can do Tag#tagKey(), but I figured a method to get all TagKeys would be a nice addition.
(sorry, Im new to using paper's tag/registry stuff and I might be rambling)

@Machine-Maker
Copy link
Member Author

Yeah, any such API method would just be doing the getTags().map(Tag::key) internally anyways. There isn't a nice way to get all the tag keys without doing that map. So probably wont add that, and I'll just suggest doing getTags().map(Tag::key) to get a Stream<TagKey<T>>.

@ShaneBeee
Copy link

Yeah, any such API method would just be doing the getTags().map(Tag::key) internally anyways. There isn't a nice way to get all the tag keys without doing that map. So probably wont add that, and I'll just suggest doing getTags().map(Tag::key) to get a Stream<TagKey<T>>.

Ok yeah thats fair. Not terribly difficult on the users end.
Thanks :)

@kennytv
Copy link
Member

kennytv commented Dec 25, 2024

It feels weird to return a stream here since it's just arbitrary internal design that happens to return a Stream, I would just toList it or skip the stream and directly iterate over the bound TagSet to throw it into a List/Collection. Performance shouldn't be of any concern there

@Machine-Maker Machine-Maker force-pushed the feature/Registry#getTags branch from 2d11a19 to bb4981a Compare December 25, 2024 21:43
@ShaneBeee
Copy link

ShaneBeee commented Dec 25, 2024

Sorry to sound like a total pain in the rear end here...
Since you're now returning a Collection instead of a stream, getting keys would mean stream(internal) -> list -> stream again (on my end) (and map) and then back to a list...
So with that said, maybe now is a good time to add a getTagKeys method?
(since internally you could do the map before toList return)

@Machine-Maker
Copy link
Member Author

Yeah, I asked about that, but there isn't really any API that consumes a collection of tag keys, so just make the stream yourself anyways. This shouldn't really be any sort of performance thing, tags don't change except on minecraft:reloads so checking constantly shouldn't be an issue.

@Machine-Maker Machine-Maker merged commit 3a479ea into PaperMC:main Dec 26, 2024
3 checks passed
@Machine-Maker Machine-Maker deleted the feature/Registry#getTags branch December 26, 2024 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Request for a new Feature.
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

Addition of Registry#getTags
3 participants