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 multilingual RDF #124

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

Conversation

stefina
Copy link
Member

@stefina stefina commented Jul 17, 2018

No description provided.

@seitenbau-govdata
Copy link
Member

Hi @stefina,
we from the German open data portal are also interested in the multilingual support.

Is this the final implementation or is something missing? It seems that the actual change in the pull request changes the type of the expected default value if no value is in the graph to the given predicate. I think therefore some of the unit tests are failing, because the return value is different.

As @amercader mentioned in #55 there is a need to use ckanext-scheming for enabling proper storing of the multilingual fields in the database. A minimal example for the fields title and notes should be could be helpful to get started with it.

@stefina stefina changed the title WIP: add support for multilingual RDF add support for multilingual RDF Sep 10, 2018
@stefina
Copy link
Member Author

stefina commented Sep 10, 2018

@seitenbau-govdata I added some documentation to make it clear how this is used. Would be good to hear your opinion on it.
Unfortunately the code-coverage-tests fail since I added documentation.
@amercader I think it makes sense to exclude the readme (or *.md-files altogether). What do you think? ;)

@seitenbau-govdata
Copy link
Member

@stefina It looks good to me. Maybe a hint would be helpful that the plugin ckanext-scheming is needed to store the values in several languages in the database properly. As I have understand the ckanext-scheming is required then, right?

Copy link
Member

@amercader amercader left a comment

Choose a reason for hiding this comment

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

@stefina many thanks for this really powerful feature, and thanks @seitenbau-govdata for the feedback.

For the record, the extension that handles multilingual fields is ckanext-fluent (generally used alongside ckanext-scheming), so this should be mentioned in the docs.

IIUC, with this implementation people would need to create their own profiles, and in them for each field that needs to be extracted, call _object_value with multilang=True. Does it make sense to have a global flag (passed when creating the profile instance or controlled via config option) to enable multilingual support globally? Or perhaps is better to control which individuals fields are handled in this way?

I just don't know enough about the use case or how multilingual metadata is encoded in RDF (ie are all text fields generally translated, just a subset, etc) so your view on this are appreciated.

return ''

def _add_multilang_value(self, subject, predicate, dataset_key, dataset_dict): # noqa
Copy link
Member

Choose a reason for hiding this comment

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

It was not initially clear what this function did, would you mind adding a short docstring along the lines of "this function will add a new triple for each language defined in a multilingual field dict, eg ..."

Actually I'm wondering if the logic in this function could be incorporated to the existing helpers to add triples directly, eg here so it is handled automatically on existing profiles without having to write your own and use _add_multilang_value()

@@ -80,6 +80,33 @@ def test_object_value_not_found(self):

eq_(value, '')

def test_object_value_multilang(self):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a small test for the other way around (ie from dict to graph)?

@stefina stefina force-pushed the 55-support-for-multilingual-rdf branch from aa0a109 to 1920630 Compare October 12, 2018 11:04
@stefina stefina force-pushed the 55-support-for-multilingual-rdf branch 2 times, most recently from 433702e to ad1b589 Compare October 12, 2018 11:15
@stefina stefina closed this Oct 15, 2018
@stefina stefina reopened this Oct 15, 2018
@stefina stefina force-pushed the 55-support-for-multilingual-rdf branch from ad1b589 to e1418da Compare October 16, 2018 11:44
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