hellemo / ShortCodes.jl

Short codes to embed content in Pluto notebooks
MIT License
49 stars 4 forks source link

Fallback support for failed metadata retrieval (Fixes #13). #14

Closed stefanbringuier closed 11 months ago

stefanbringuier commented 1 year ago

Summary

This PR adds a new function called metadata_template(doi) which returns a dictionary with fields corresponding to the OpenCitations metadata structure from the new API (https://w3id.org/oc/meta/api/v1/metadata). The function populates the values with null strings except for the id field, which is given the DOI::String

Additionally, the fetch_metadata function has been updated with an if-else clause that checks if the returned JSON from OpenCitations is empty for a given DOI. If empty, the new metadata_template function is called.

No changes were made to the Base.show methods, but the year function now uses a ternary operator condition to return null string if empty. This supports Base.getproperty call to year.

This PR addresses issue #13.

Changes

Testing

No new tests were added, but existing tests pass.

Notes

Please review the changes and let me know if you have any modifications or design pattern changes.

AshtonSBradley commented 1 year ago

Great to see this progress!

I hope I am testing this correctly. Running some of the examples from #13:

Screenshot 2023-04-09 at 5 10 09 PM

Leads to, e.g.

Screenshot 2023-04-09 at 5 11 04 PM
stefanbringuier commented 1 year ago

@AshtonSBradley Thanks for testing a few cases. I had set the metadata_template returned dictionary values to null character rather than an empty string, but the condition in year only checked for an empty string because in testing other DOIs there were some which showed metadata but with pub_date as an empty string.

I therefore updated the condition in the year function to check this. I tested this with Pluto and seems to work now:

image

Let me know if you find anything else.

AshtonSBradley commented 1 year ago

This seems to solve it - errors are now links with cites.

stefanbringuier commented 1 year ago

This seems to solve it - errors are now links with cites.

Do you have any proposed approach to format the display for the fallback when no data is present? I decided not to add a new Base.show method and to just fallaback on the existing methods.

AshtonSBradley commented 1 year ago

This seems to solve it - errors are now links with cites.

Do you have any proposed approach to format the display for the fallback when no data is present? I decided not to add a new Base.show method and to just fallaback on the existing methods.

It appears that the only data we have in these cases is the doi and the citation count. In some cases the doi is informative, e.g. in physical review journals. I would propose formatting the doi string as the url, followed by the citation count string, e.g:

DOI("10.1103/PhysRevA.107.010101")

currently formatted to

, () 10/cs5p5f, cited by 1931

would become

10.1103/PhysRevA.107.010101, cited by 1931

AshtonSBradley commented 1 year ago

A related comment (off topic slightly, apologies), I wonder about simplifying the current formatting that provides a short link. Why not just mark up either the journal title or the article title to be the link, as done by some bibtex styles. What I mean is, e.g. currently:

DOI("10.1098/rsta.2010.0333") returns

Garraway, B M The Dicke Model In Quantum Optics: Dicke Model Revisited, Philosophical Transactions Of The Royal Society A: Mathematical, Physical And Engineering Sciences (2011) 10/c993k5, cited by 254

but this can be clarified and shortened to

Garraway, B M The Dicke Model In Quantum Optics: Dicke Model Revisited, Philosophical Transactions Of The Royal Society A: Mathematical, Physical And Engineering Sciences (2011), cited by 254

or

Garraway, B M The Dicke Model In Quantum Optics: Dicke Model Revisited, Philosophical Transactions Of The Royal Society A: Mathematical, Physical And Engineering Sciences (2011), cited by 254

Personally I prefer the former as it makes the article content more evident

stefanbringuier commented 1 year ago

... would become

10.1103/PhysRevA.107.010101, cited by 1931

I would agree this would look the best in my opinion. I'll wait to see if @hellemo wants to keep with this PR or go a different direction before making changes to the Base.show methods.

hellemo commented 1 year ago

Thanks for the PR, and sorry for not following up earlier.

I think this looks like a reasonable approach. One suggestion for a change I have is to use either an empty string "" or perhaps nothing rather than the string "\0".

For the suggested formatting change, I think that would be an improvement, so would be happy to accept a PR implementing that as well. For a bit of context, the idea of using the short doi was originally to have a very short textual (without hyperlink) reference to the paper, when copying from the pluto notebook to use in another document. But I guess very few potential readers would actually understand what to do with this short code, so linking the title seems more useful to me.

stefanbringuier commented 1 year ago

I think this looks like a reasonable approach. One suggestion for a change I have is to use either an empty string "" or perhaps nothing rather than the string "\0".

No problems will implement.

For the suggested formatting change, I think that would be an improvement, so would be happy to accept a PR implementing that as well. For a bit of context, the idea of using the short doi was originally to have a very short textual (without hyperlink) reference to the paper, when copying from the pluto notebook to use in another document. But I guess very few potential readers would actually understand what to do with this short code, so linking the title seems more useful to me.

For now, I'll just focus on handling the case where no metadata is present and return the full DOI with citations.

AshtonSBradley commented 1 year ago

Hi, just wondering if there is a problem with the missing metadata pr

stefanbringuier commented 1 year ago

Hi, just wondering if there is a problem with the missing metadata pr

I haven't followed-up on this pr, let me revisit and make any commits.

AshtonSBradley commented 11 months ago

Is there any reason this can't be merged? ShortCodes is unusable for doi fetching if it errors for incomplete metadata because any notebook with an incomplete return will appear to hang for about 2 minutes before returning an error

hellemo commented 11 months ago

Is there any reason this can't be merged? ShortCodes is unusable for doi fetching if it errors for incomplete metadata because any notebook with an incomplete return will appear to hang for about 2 minutes before returning an error

Thanks for the reminder, it was the empty string thing, and I since forgot about it. Will merge and test and prepare a new release soon.