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 translator for Libris #3184

Merged

Conversation

sebastian-berlin-wmse
Copy link
Contributor

Libris is a service by The National Library of Sweden ("Kungliga biblioteket"). It collects information from several Swedish libraries. For more information see
http://libris.kb.se/help/about_libris_eng.jsp?language=en.

This translator is based on the translator for National Library of Poland ISBN. Based on the discussion in #3036 it filters only books published in Sweden. It uses the Xsearch API:
http://libris.kb.se/help/xsearch_eng.jsp?language=en.

Libris is a service by The National Library of Sweden ("Kungliga
biblioteket"). It collects information from several Swedish
libraries. For more information see
http://libris.kb.se/help/about_libris_eng.jsp?language=en.

This translator is based on the translator for National Library of
Poland ISBN. Based on the discussion in zotero#3036 it filters only books
published in Sweden. It uses the Xsearch API:
http://libris.kb.se/help/xsearch_eng.jsp?language=en.
@sebastian-berlin-wmse
Copy link
Contributor Author

I'm not sure why the tests failed. Looks like maybe they were timed out? I found another recent PR where they failed after 10 minutes: https://github.com/zotero/translators/actions/runs/6708719978/job/18230143937?pr=3173.

Is there a way to re-run the tests?

@adam3smith
Copy link
Collaborator

re-running won't fix the tests -- this looks like it's getting stuck running the tests and then just aborting after 10mins. Looks good to me otherwise, I'm glad to see more localized ISBN import!

@sebastian-berlin-wmse
Copy link
Contributor Author

Does that mean the test fails in the CI? I can't tell from the logs and it passes locally for me.

@adam3smith
Copy link
Collaborator

Right, the CI tries to not just test linting but also run the translator test in an emulated environment and that just seems broken here and possibly for search translators more generally. That should get fixed, but it's not anything you need to worry about.

@sebastian-berlin-wmse
Copy link
Contributor Author

I just want to doublecheck so I haven't missed anything: is there anything I need to do before this can be reviewed?

Libris.js Outdated Show resolved Hide resolved
@AbeJellinek
Copy link
Member

Thanks, looks good, just one nitpick.

@AbeJellinek
Copy link
Member

Oh, and some naming nitpicks:

  • It looks like the site calls itself LIBRIS, not Libris.
  • We should follow our naming convention for other ISBN translators and call this "LIBRIS ISBN".

@sebastian-berlin-wmse
Copy link
Contributor Author

Tanks for the review.

@sebastian-berlin-wmse
Copy link
Contributor Author

Is there anything else that needs to be done before this can be merged?

@AbeJellinek AbeJellinek merged commit b3dd2b4 into zotero:master Apr 23, 2024
1 check passed
@AbeJellinek
Copy link
Member

Thank you!

@sebastian-berlin-wmse sebastian-berlin-wmse deleted the add-translator-for-libris branch April 24, 2024 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants