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

Introduced visitors into datamodel and JSON converter #55

Merged
merged 5 commits into from
Apr 5, 2014

Conversation

mkroetzsch
Copy link
Member

  • New interfaces ValueVisitor and SnakVisitor can be used to avoid
    casting objects of these types
  • Renamed ConverterImpl to JsonConverter; restricted public interface to
    methods for handling EntityDocuments; removed Converter interface as it
    is not needed
  • Split JsonConverter into several classes by extracting the respective
    visitors
  • Extracted constants from JsonConverter
  • Renamed "jsonconverter" package to "json"
  • Updated tests accordingly

A lot of code has been moved and renamed in this commit. Although much of the moved code is unchanged, the diff is probably not of much use. Best approach for review might be to just ignore the diff and review the changed files as they are now (the changes in datamodel.implementation are trivial; the main differences are in the json package).

* New interfaces ValueVisitor and SnakVisitor can be used to avoid
casting objects of these types
* Renamed ConverterImpl to JsonConverter; restricted public interface to
methods for handling EntityDocuments; removed Converter interface as it
is not needed
* Split JsonConverter into several classes by extracting the respective
visitors
* Extracted constants from JsonConverter
* Renamed "jsonconverter" package to "json"
* Updated tests accordingly
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) when pulling bd3b396 on datamodel-visitors into fdc5e72 on master.

This interface is not specific to dumpfiles. For example, it might be
useful for implementing a class that writes serialized JSON to some
stream.
Seems that this does not get updated by the Maven plugin.
@guenthermi
Copy link
Member

I review your code. Your code looking good, but I realized a bug form me side. There is no entry for "qualifiers-order" after the SnakGroup for the qualifiers. Should I make this changes on this branch?

@mkroetzsch
Copy link
Member Author

You can also merge this first and make the changes later. It is not really related to the introduction of visitors here. Once this change is merged, I can also try to make #57 mergeable.

Conflicts:
	wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/SomeValueSnakImpl.java
guenthermi added a commit that referenced this pull request Apr 5, 2014
 	Introduced visitors into datamodel and JSON converter
@guenthermi guenthermi merged commit c2ce511 into master Apr 5, 2014
fer-rum pushed a commit that referenced this pull request Apr 16, 2014
Sitelink support (merge only after #55)
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