Capitains / MyCapytain

Texts API and Textual Resources Utility Library for Python 3
http://mycapytain.readthedocs.org
Mozilla Public License 2.0
8 stars 10 forks source link

JSON.Std serialization of metadata does not deal with multiple values #187

Closed sonofmun closed 5 years ago

sonofmun commented 5 years ago

https://github.com/Capitains/MyCapytain/blob/b11bbf6b6ae141fc02be70471e3fbf6907be6593/MyCapytain/common/metadata.py#L201-L210 After line 206, there should be an else statement that will add another value to the key as opposed to the key simply being ignored.

PonteIneptique commented 5 years ago

I am definitely not sure of that

PonteIneptique commented 5 years ago

Damn, clicked too fast. Let me explain myself here : line 206 actually adds the predicate to the out dictionary. An else would not change that I think. But maybe you meant line 208 ?

sonofmun commented 5 years ago

Well, it needs to be changed differently. But if you look, if two predicates have the same language, one will overwrite the other. But this should not be the case. You could have two editors whose names are both in German. So there should be multiple values for editor in German.

PonteIneptique commented 5 years ago

Ok, I think I get you. But I think it's a lot more no ?

         if isinstance(object, Literal): 
             if object.language in out[predicate]:
                 if isinstance(out[predicate][object.language], str):
                     out[predicate][object.language] = [out[predicate][object.language]]
                 out[predicate][object.language].append(object.title() )
           else:
               out[predicate][object.language]
sonofmun commented 5 years ago

Exactly. That is what I was thinking. The problem could be that we will have some values that will simply be literals and some that will be arrays (or whatever they are called in JSON). Will that cause a problem somewhere down the line, e.g., with metadata.get_single?

sonofmun commented 5 years ago

Although that pulls things directly from the graph, so that should not be a problem.

sonofmun commented 5 years ago

I will work on a PR for this later today.

sonofmun commented 5 years ago

Fixed in PR #188

sonofmun commented 5 years ago

I assume it will show up on PyPi automagically now that I have released.