Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Processor result object #8
Processor result object #8
Changes from 2 commits
5117684
456cc6d
e03a906
90afb8a
3d094d6
72eb75b
75cb20c
9a1c7ad
60ad424
50dfdd6
53f2634
d210afa
5718cf9
f5f3145
db68bb5
7045318
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why
Optional
(also in function prototype)?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.
Here: Because we're instantiating a list of
None
values, which are notOcrdPage
.In the function signature of
process_page_pcgts
: Same situation, there might be "holes" in the list ofinput_pcgts
when any of theinput_files
inprocess_page_files
cannot be parsed as PAGE-XML.And for
process_page_files
: Theinput_files
can be hole-y, if theworkspace.download_file
fails for any of the files (beyond the first?).But really, I was trying to make sure that static type checking had no more complaints. I tried to add
assert
statements where I know that variables must be defined or of a certain type to mitigate the "everything might beNone
" problem somewhat.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.
Oh, right, I forgot about the holes returned by zip_input_files for multiple fileGrps but incomplete PAGE-XML coverage per page!
Maybe we should document this more loudly.
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.
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.
Can this ever happen, ie. can
page_from_file(with_etree=False)
ever return anything other than aPcGtsType
? I think if that was ever the case, we'd want thatAssertionError
to be raised because then we'd have broken something.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.
You're right – it cannot happen. But then what is the assertion good for – satisfying the type checker?
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.
First, my curiosity that I understand the behavior correctly. But secondly, yes, the type checker ;)
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.
But reading this again, I should have used
OcrdPage
notPcGtsType
, which is just an alias but we useOcrdPage
in the method typing.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.
Yes. Feel free to change in OCR-D#1240.