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 helper methods to CustomValue and its implementations #338

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

BoogieMonster1O1
Copy link

@BoogieMonster1O1 BoogieMonster1O1 commented Nov 25, 2020

CustomValue's implementations do not implement equals or hashCode. This causes issues when storing them in lists and maps, where it is impossible to remove CustomValues from a list, or get the value associated with a CustomValue key from a map.
This also affects any DynamicOps relying on CustomValues as FieldDecoders get returned null when reading a CustomValue keyed map.

This makes CustomValue's implementations implement equals, hashCode and toString.
This also adds a stream method to CvObject and CvArray, and a keySet method to CvObject for convenience.

@liach liach added the bug label Jun 24, 2021
@liach
Copy link
Contributor

liach commented Jun 24, 2021

The custom value's status as not being value-based should be a bug. @sfPlayer1 What's your view on this whole thing? Will you include the new utilities besides fixing the value-based nature of these classes?

@liach
Copy link
Contributor

liach commented Jun 30, 2021

@sfPlayer1 Even though the addition of stream methods are actually optional (as they can always stream an iterable), I wonder if we want to treat our custom values as data classes (as opposed to identity objects as they are right now)

@sfPlayer1
Copy link
Contributor

Not sure, it's really just a simple way to expose the data from the mod jsons. Implementing toString has some benefits in the debugger, but uses that depend on equals/hashCode having specific semantics are unclear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants