feat: add type handlers for SimpleUri and BlockUri

[jdrueckert]

Contains

New type handlers to support de-/serializing to/from SimpleUri and BlockUri.

How to test

Can be tested in combination with Terasology/ChangingBlocks#15.

[keturn]

Lets add a check of uri.isValid() here. It's a cheap check, and doing it here instead of leaving it until later will make it easier to identify what the source string is and where it came from.

As for what we do if it doesn't pass the check... uh, if we think the deserialization code is trying to avoid throwing exceptions, I guess we log the error but still return the invalid SimpleUri instance?

[skaldarnar]

Looking at the implementation of StringRepresentationTypeHandler returning null would yield Optional.empty, which is our equivalent to "could not parse"... That probably better than returning an invlid SimpleUri...

Terasology/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java

Line 22 in b367a18

return Optional.ofNullable(getFromString(data.getAsString()));

[skaldarnar]

Suggested change

	logger.error("Failed to create valid SimpleURI from string '{}'", representation);
	logger.error("Failed to create valid SimpleURI from string '{}'", representation);
	// StringRepresentationTypeHandler will turn this 'null' value into an empty Optional
	return null;

[jdrueckert]

done ✔️

also added tests

[keturn]

[Oops, duplicate comment.]

[keturn]

I think this looks good.

I was thinking a bit about what kind of test would be appropriate. I don't think SimpleUriTypeHandler would benefit from any additional tests to its methods, but maybe we could add an integration test one level higher up that demonstrates we can use SimpleUri with the default TypeHandlerLibrary? Would that be useful?

[Approving but not merging yet because the Release Candidate has already been cut and I'm not clear on which things need to go in this release.]

[jdrueckert]

maybe we could add an integration test one level higher up that demonstrates we can use SimpleUri with the default TypeHandlerLibrary? Would that be useful?

Could be but I'd argue that that would be a suitable consideration for the upcoming type handler improvements when we look at the contracts of the type handlers.

[jdrueckert]

As discussed in the contributor meeting, I added a type handler for BlockUri.

requested changes added

[keturn]

one small correction; feel free to merge after adding that if it works with your ChangingBlocks use case.

[keturn]

We'll want to add BlockUri's handler here too, right?

[jdrueckert]

good catch, thank you! will add that later today 👍

[jdrueckert]

one small correction; feel free to merge after adding that if it works with your ChangingBlocks use case.

correction added and tested, works fine for the ChangingBlocks use case using ChangingBlocks#15 and PlantPack#20 👍

correction added