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

[FormRecognizer] Fix an issue accessing undefined object #8748

Merged
merged 4 commits into from
May 15, 2020

Conversation

jeremymeng
Copy link
Member

when input document to receipt recognition contains blank pages

@jeremymeng jeremymeng requested a review from willmtemple as a code owner May 6, 2020 23:20
@jeremymeng
Copy link
Member Author

This should wait for #8747 to be merged first.

@jeremymeng jeremymeng added Client This issue points to a problem in the data-plane of the library. FormRecognizer labels May 6, 2020
when input document to receipt recognition contains blank pages
@jeremymeng jeremymeng force-pushed the fr-handle-blank-pages branch from 9cb5687 to c894276 Compare May 7, 2020 05:22
@jeremymeng
Copy link
Member Author

/azp run js - formrecognizer - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jeremymeng
Copy link
Member Author

Part of #8706

also added back confidence for receipt type.
@jeremymeng jeremymeng requested a review from bterlson as a code owner May 11, 2020 19:08
return "itemized";
} else {
return "unrecognized";
if (type.valueType === "string") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I get a little bit spooked out by this block. The type signature suggests that "gas" and "parking" are also valid values of type, and it feels like we're doing too much to transpose FormField into USReceiptType here. Is there any reason we couldn't return type.valueType in all cases where it's defined and then use "unrecognized" otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed the confidence field on receipt type, it will be included in another PR so we will be returning `{ confidence: 1, type: "itemized" }

Copy link
Member Author

Choose a reason for hiding this comment

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

oh it's in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

So the valueType for a ReceiptType field should be "string" and the value of the field type.value is the receipt category, which is capitalized and I want to transform it into the camelCase that we normally use in TypeScript.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we are fine with capitalized string literals for receipt types then yes we can return them directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm returning type.value which has a string type as stronger receipt type might not be valid here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like TypeScript compiler can correctly narrow down the to the valid string literals. I refactored this to use a switch statement. Hopefully it reads better.

Copy link
Contributor

@willmtemple willmtemple left a comment

Choose a reason for hiding this comment

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

Cool, I like the tweak with the switch statement.

@jeremymeng
Copy link
Member Author

/azp run js - formrecognizer - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jeremymeng
Copy link
Member Author

/azp run js - formrecognizer - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jeremymeng jeremymeng merged commit c10c9ff into Azure:master May 15, 2020
@jeremymeng jeremymeng deleted the fr-handle-blank-pages branch May 15, 2020 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Form Recognizer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants