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

[form recognizer] Remove US receipt #11764

Merged
merged 12 commits into from
Jun 3, 2020

Conversation

iscai-msft
Copy link
Contributor

fixes #11712

Copy link
Member

@kristapratico kristapratico left a comment

Choose a reason for hiding this comment

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

Looks good, mostly just want to hear back about receipt_type before approving

sdk/formrecognizer/azure-ai-formrecognizer/CHANGELOG.md Outdated Show resolved Hide resolved
sdk/formrecognizer/azure-ai-formrecognizer/README.md Outdated Show resolved Hide resolved
print("Tip: {} has confidence: {}".format(tip.value, tip.confidence))
total = receipt.fields.get("Total")
if total:
print("Total: {} has confidence: {}".format(total.value, total.confidence))
Copy link
Member

Choose a reason for hiding this comment

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

I understand wanting to show the keys available, but wondering if we should do at least one sample or readme like this:

for receipt in result:
    for key, val in receipt.fields.items():
        print("{}: {} has confidence: {}".format(key, val.value, val.confidence))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean. I think I will leave the samples like this, but update the readme sample to traverse the fields like this

Copy link
Member

Choose a reason for hiding this comment

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

I really like how the readme looks.
For this sample, I will suggest adding a comment on top a link to the types the service is currently supporting, or guiding the user into finding this information

Copy link
Member

Choose a reason for hiding this comment

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

For the .NET sample, we're only showing a subset of illustrative values from the receipt.

Also, I'm not sure how it works in Python, but do you need to check the FieldValueType to cast these to the right time in order for a customer to use them in a scenario?

@iscai-msft iscai-msft requested a review from kristapratico June 3, 2020 15:28
maririos
maririos previously approved these changes Jun 3, 2020
Co-authored-by: Krista Pratico <[email protected]>
@iscai-msft iscai-msft requested a review from maririos June 3, 2020 16:40
for name, field in receipt.fields.items():
if name == "Items":
print("Receipt Items:")
for idx, items in enumerate(field.value):
Copy link
Member

Choose a reason for hiding this comment

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

If it's at all complicated to retrieve these values, it's probably worth illustrating for customers how to get them out of the receipt fields property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are still accessing the field properties (including items) one by one in the samples. For the readme, based on @kristapratico , I wanted to show a simpler way of traversing everything

@iscai-msft iscai-msft merged commit 3094344 into Azure:master Jun 3, 2020
@iscai-msft iscai-msft deleted the remove_us_receipt branch June 3, 2020 20:23
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.

[form recognizer] remove USReceipt
4 participants