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

Improve overall type safety #18

Merged
merged 17 commits into from
Dec 20, 2018
Merged

Improve overall type safety #18

merged 17 commits into from
Dec 20, 2018

Conversation

Querz
Copy link
Owner

@Querz Querz commented Dec 12, 2018

See #14
ListTag should remember its type when cleared.

The current solution makes the no-args constructor of ListTag protected so deserialisation can still create an instance without knowing the type of the ListTag. Everything from outside creating a new instance of ListTag will need to use ListTag(Class<T extends Tag>).

I also modified ListTag#getTypeID() and ListTag#getTypeClass() to match the old behaviour and be consistent with serialisation. Another reason for this is that these methods cannot be consistent with each other when the ListTag is empty.
Example:

ListTag<IntTag> l = new ListTag<>(IntTag.class);

can only set the type class of the ListTag, but not the type id. Setting the type id occurs when adding the first element to the new ListTag.
Therefore ListTag#getTypeID() will return 0 and ListTag#getTypeClass() returns EndTag.class when the ListTag is empty.


Other cases that will fail now but worked before:

ListTag<IntTag> l = new ListTag<>(IntTag.class);
l.addByte((byte) 123); // will now throw an IllegalArgumentException
ListTag<IntTag> l = new ListTag<>(IntTag.class);
l.addInt(123);
l.clear();
l.addByte((byte) 123); // will now throw an IllegalArgumentException

The behaviour of ListTag#asTypedList(Class<? extends Tag>) also matches the new behaviour.


All Unit Tests have been adjusted.

@coveralls
Copy link

coveralls commented Dec 12, 2018

Coverage Status

Coverage increased (+1.4%) to 98.962% when pulling c821420 on listtag-type into 947c750 on master.

Querz and others added 4 commits December 12, 2018 14:10
- Made no-args non-type-safe constructor private and added protected method instead to prevent accidential usage
- Fixed being able to create EndTag lists (since they are not type-safe)
- Fixed empty list and list.add().clear() not being equal due to typeID difference
- Fixed clone() creating non-type-safe list if this list is empty
@Querz Querz changed the title Make LisTag type more persistent Improve overall type safety Dec 13, 2018
@Querz Querz mentioned this pull request Dec 13, 2018
@Querz
Copy link
Owner Author

Querz commented Dec 14, 2018

The last commit removes the typeID from ListTag and changes the TagFactory to work with this.
ListTag only uses the type class now. We can also look into using it with an Optional. I don't mind using it, i just didn't have the time for it yet.

- tag ids should only be in one place
- correct wrong ListTag class java-doc
- unchecked empty list toString() throws NPE
@Querz
Copy link
Owner Author

Querz commented Dec 15, 2018

Maybe an unchecked (and therefore empty) list should always be equal to any other empty list, but then hashCode must not include the type class anymore when the list is empty.

If not then at least an unchecked list should be equal to another unchecked list.

Behaviour now is that ListTag#equals() will return true if both ListTags are either empty AND untyped or empty and with the same type or not empty, with the same type and same values in the same order.

ListTag#hashCode() matches this behaviour by including the type of the ListTag.


Another thing i noticed which is not very important but i'd like to change is the way ListTag#toString() prints the values by using the full #toString() on each tag, which includes the type of each tag. This information is redundant as their type is already printed in the "type" field.
The result of ListTag#toString() should be something like this:

{
	"type": "ListTag",
	"value": {
		"list": [
			-128,
			0,
			127
		],
		"type": "ByteTag"
	}
}

@Querz Querz merged commit 66f0d0d into master Dec 20, 2018
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.

None yet

3 participants