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

handle oai response with workspace clone #581

Merged
merged 10 commits into from
Sep 2, 2020

Conversation

M3ssman
Copy link
Contributor

@M3ssman M3ssman commented Aug 28, 2020

New Feature to enable workspace creation from OAI-Responses.
See #539 for details.

@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2020

Codecov Report

Merging #581 into master will increase coverage by 0.10%.
The diff coverage is 93.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #581      +/-   ##
==========================================
+ Coverage   84.60%   84.71%   +0.10%     
==========================================
  Files          49       49              
  Lines        2813     2845      +32     
  Branches      550      554       +4     
==========================================
+ Hits         2380     2410      +30     
- Misses        332      333       +1     
- Partials      101      102       +1     
Impacted Files Coverage Δ
ocrd/ocrd/cli/workspace.py 76.47% <ø> (ø)
ocrd_models/ocrd_models/utils.py 94.28% <93.10%> (-5.72%) ⬇️
ocrd/ocrd/resolver.py 96.66% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8dafbac...02712ab. Read the comment docs.

Copy link
Member

@kba kba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you very much. I like how concisely you implemented this and I've learned about pytest/unittest features I haven't used yet but will in the future.

I have refactored a few things and from my POV this can be merged.

However, I would really like to see some tests with real-world OAI-PMH endpoints to make sure the solution is robust enough. @wrznr @mikegerber can you try this and give short feedback whether this PR fits your use case?

@wrznr wrznr self-requested a review August 28, 2020 14:32
@kba
Copy link
Member

kba commented Aug 28, 2020

I've tested this with a few samples provided by @wrznr @maria-federbusch and @cneud from OAI-PMH endpoints at @slub and @StaatsBibliothekBerlin and they all work flawlessly

@kba kba merged commit 02712ab into OCR-D:master Sep 2, 2020
@kba kba deleted the feature/handle-oai-response branch September 3, 2020 13:29
@mikegerber
Copy link
Contributor

However, I would really like to see some tests with real-world OAI-PMH endpoints to make sure the solution is robust enough. @wrznr @mikegerber can you try this and give short feedback whether this PR fits your use case?

Sorry for not replying earlier: It's working fine 👍 (The METS needs more massaging after that, i.e. #544, but that's a different issue.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants