-
Notifications
You must be signed in to change notification settings - Fork 53
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
Expose attachments #99
base: master
Are you sure you want to change the base?
Conversation
Given the test output, I wonder if a better approach would be to embed the attachments as fields of the record, as originally stored in Zotero? |
Hey, thanks for doing the pullrequest!
On Sun, Mar 24, 2019 at 11:10:21AM -0700, Antonin Delpeuch wrote:
Given the test output, I wonder if a better approach would be to embed
the attachments as fields of the record, as originally stored in
Zotero?
As stated in the original issue, the reason for the current format is
that I tried to represent attachments in a similar fashion to the note
handling in the export. Those are regular item-members as well, but are
represented as dedicated items with a parent relation within the export.
I'm not sure what the primary intention was, but maybe one of the
developers can step in there.
|
@dstillman any preference about the desired output for this? Would you merge this if we update the tests? |
Any updates on this? |
I've now adapted the tests and fixed one minor flaw in the current implementation (not all url-attachments actually have a title). |
Generally, it is not useful to expose regular Zotero attachments via the API. However, url attachments are an exception: url resources are of use to the api user, as they help locating various linked online artifacts that can be readily accessed (in contrast to other attachment types residing within Zotero's store). Therefore, this commit exports them as linked items into the API-response.
The previous commit introduced url attachments, which have to be corretly handled/expected within the tests, which this commit rectifies.
@wetneb Thanks! |
Any blockers here, or is this just pending a pull? e.g. why hasn't this been pulled? Thansk |
Just out of curiosity: @dstillman: Any new input on this? Either a definite yes or no? Would it help to make this configurable (with a default to false) to make it more acceptable to publically hosted instances or something like that? Do you want changes to be made to the output format? |
This is the patch suggested by @noctux in #97. It would be a very useful addition to translation-server, I think.