-
Notifications
You must be signed in to change notification settings - Fork 25
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
documents: simplify subjects structure #3278
Conversation
3e373c3
to
aac5d59
Compare
build_string_from_subfields( | ||
value, | ||
subfield_code_per_tag[creator_tag_key]), | ||
'.', '.' | ||
) | ||
) + '. ' + subject['authorized_access_point'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't understand why we have to add subject['authorized_access_point']
again.
If we really have to add it it is preferable to work with f
strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rerowep in this case (600t, 610t, 611t), it seems some subfields value must be prepend to subject term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we really have to add it it is preferable to work with
f
strings.
I do not see a simple way to do this. Can you provide an example?
@@ -0,0 +1,59 @@ | |||
{ | |||
"title": "Local Entity", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to name Local Contribution
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to express the fact that a contribution is an entity. The professional should "see" that an entity can be a subfield of different document fields.
} | ||
] | ||
} | ||
"title": "Entities", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File name and title not matching ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No but should it be?
@@ -0,0 +1,60 @@ | |||
{ | |||
"title": "Local Entity", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File name and title not matching ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No but should it be?
}, | ||
"place_subdivisions": { | ||
"$ref": "#/definitions/place_subdivisions" | ||
"title": "Entities", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File name and title not matching ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No but should it be?
# print(str(err)) | ||
return 'Subject parsing error !' | ||
entity = subject_data['entity'] | ||
if language is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally we don't need this test, because entity.get(...
will do the job, but it is more clear this way.
build_string_from_subfields( | ||
value, | ||
subfield_code_per_tag[creator_tag_key]), | ||
'.', '.' | ||
) | ||
) + '. ' + subject['authorized_access_point'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rerowep in this case (600t, 610t, 611t), it seems some subfields value must be prepend to subject term.
'z': 'bf:Place' | ||
} | ||
for tag, val in value.items(): | ||
if tag in subdivisions.keys(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COSMETIC : you could use if tag in subdivisions:
. Using this notation on a dict, will check into dict.keys()... but it's maybe less human readable.
for subject in [ | ||
res.get('entity') for res in doc.get(self.name, []) | ||
]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COSMETIC : maybe add a default value for res.get('entity', {})
. It should prevent any error if entity
isn't present (I know it's required into JSON schema... but 2 security systems are always better than only one)
'entity': { | ||
'$ref': 'n/a', | ||
'pid': 'n/a', | ||
'type': DocumentSubjectType.PERSON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use EntityType
class and remove DocumentSubjectType
as we already removed subjectFactory
"items": { | ||
"type": "object", | ||
"title": "Entities", | ||
"description": "Topic (including genre/form), place, temporal, person, family or corporate body (including conferences). Always create a link to IdRef or GND, if possible.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Imported topic, place, temporal, person, family or corporate body (including conferences), used for information. Do not enter data manually."
"items": { | ||
"type": "object", | ||
"title": "Entities", | ||
"description": "Topic (including genre/form), place, temporal, person, family or corporate body (including conferences). Always create a link to IdRef or GND, if possible.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Imported genre or form, used for information. Do not enter data manually."
"cssClass": "editor-title" | ||
} | ||
"title": "Genres, forms (imported)", | ||
"description": "Genre of form from imported bibliographic data", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Genre or form imported from external data"
"cssClass": "editor-title" | ||
} | ||
"title": "Subjects (imported)", | ||
"description": "Subject from imported bibliographic data", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Subject imported from external data"
}, | ||
"source": { | ||
"title": "Source", | ||
"description": "Source of the subject.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Source catalog where the subject was imported from."
"items": { | ||
"type": "object", | ||
"title": "Entities", | ||
"description": "Topic (including genre/form), place, temporal, person, family or corporate body (including conferences). Always create a link to IdRef or GND, if possible.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Topic (including genre/form), place, temporal, person, family or corporate body (including conferences). Always create a link to IdRef or GND, if possible."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be a copy from the original description.
"bf:Topic", | ||
"bf:Concept", | ||
"bf:Place", | ||
"bf:Temporal" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bf:Person and bf:Organisation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No as subdivision does not allows Person nor Organisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so default should not be bf:Person
] | ||
} | ||
"title": "Entities", | ||
"description": "Topic (including genre/form), place, temporal, person, family or corporate body (including conferences). Always create a link to IdRef or GND, if possible.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Genre or form of the document. Always create a link to IdRef or GND, if possible."
"source": { | ||
"title": "Source", | ||
"description": "Source of the subject.", | ||
"type": "string", | ||
"minLength": 3, | ||
"form": { | ||
"templateOptions": { | ||
"itemCssClass": "col-lg-6" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed.
* Uses entity for subjects and imported subjects. * Uses entity for genreForm and imported genreForm. * Uses entities as subject subdivisions. * Removes non working entity links such as timestamps, places, etc. * Removes entity links for imported version of subjects and genreForm. * Removes subject factory. Co-Authored-by: Johnny Mariéthoz <[email protected]> Co-Authored-by: Renaud Michotte <[email protected]>
Why are you opening this PR?
Dependencies
My PR depends on the following
rero-ils-ui
's PR(s):How to test?