-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: StartRecognizeCustomFormsFromUri works when URI has blank space #12192
Conversation
sdk/formrecognizer/Azure.AI.FormRecognizer/tests/FormTrainingClient/FormTrainingClientTests.cs
Outdated
Show resolved
Hide resolved
@@ -25,6 +25,7 @@ | |||
- Custom form recognition without labels can now handle multipaged forms. | |||
- `RecognizedForm.Pages` now only contains pages whose numbers are within `RecognizedForm.PageRange`. | |||
- `FieldText.TextContent` cannot be `null` anymore, and it will be empty when no element is returned from the service. | |||
- `FormRecognizerClient.StartRecognizeCustomFormsFromUri` now works with URIs that contain blank spaces, encoded or not. |
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.
I was looking at the keyvault changelog and noticed that they put the issue number next to the fixes.
After further look, found this in the guidance: https://github.com/Azure/azure-sdk/blob/2ef0280933f7e6859360c27912bc75e5a64b0bba/docs/policies/releases.md#changelog-guidance
Could you update this section?
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.
Updated the first and last items. The second and third ones didn't have issues associated with them.
@@ -99,6 +105,33 @@ public void StartTrainingArgumentValidation() | |||
Assert.ThrowsAsync<ArgumentNullException>(() => client.StartTrainingAsync((Uri)null)); | |||
} | |||
|
|||
[Test] |
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.
Personally I see no reason for treating mock tests differently since they are just non-live tests as well. I don't feel strongly about it, so let's follow the pattern from other libraries. We can easily revert it back if we think it's not the right choice in the future.
Fixes #11564.
Adopted suggestion from #11564 (comment) and replaced every
Uri.ToString()
occurrence in the client withUri.AbsoluteUri
. Mock tests were added as well.