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 support for serialization of snak hashes #44

Merged
merged 1 commit into from
Sep 4, 2017
Merged

Conversation

lucaswerkmeister
Copy link
Member

SnakSerializer and SnakDeserializer gain support for serializing / deserializing the hash of a snak. (Serializing hashes is not required when sending serializations to the API, but the Wikibase view code assumes that serialization+deserialization is a lossless roundtrip, so if we don’t serialize it, the hash is lost when setting a new snak on a snakview.)

Copy link
Member

@manicki manicki left a comment

Choose a reason for hiding this comment

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

Seems reasonable!

Copy link
Contributor

@thiemowmde thiemowmde left a comment

Choose a reason for hiding this comment

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

The changes made in this patch are perfectly fine. But since the JavaScript DataModel classes don't support the additional hash parameter yet, this should fail. There should be at least a single, trivial test that fails right now, and succeeds the moment wmde/WikibaseDataModelJavaScript#77 is merged, released, and required via composer.json in this patch here.

I'm willing to continue working on this series of patches the next days if you don't have the time.

@lucaswerkmeister
Copy link
Member Author

lucaswerkmeister commented Aug 31, 2017

Uhm… I’m pretty sure that any test that uses SnakSerializer should fail because it calls the nonexistent method snak.getHash()… but apparently there’s no easy way to run the tests and they’re not part of CI?

@thiemowmde
Copy link
Contributor

thiemowmde commented Aug 31, 2017

You are right, the tests already fail because of the getHash call. But CI does not run the tests.

However, this patch should not be merged before the relevant change to the DataModel component is made, released, and the requirement in this components composer.json can be updated.

SnakSerializer and SnakDeserializer gain support for serializing /
deserializing the hash of a snak. (Serializing hashes is not required
when sending serializations to the API, but the Wikibase view code
assumes that serialization+deserialization is a lossless roundtrip, so
if we don’t serialize it, the hash is lost when setting a new snak on a
snakview.)
@thiemowmde thiemowmde added this to the 3.1.0 milestone Sep 4, 2017
@thiemowmde thiemowmde merged commit 13ec297 into master Sep 4, 2017
@thiemowmde thiemowmde deleted the snakHash branch September 4, 2017 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants