[FormRecognizer] Updated LROs to use new OperationInternal class#22629
[FormRecognizer] Updated LROs to use new OperationInternal class#22629kinelski merged 9 commits intoAzure:mainfrom
Conversation
kinelski
added
Client
|
/azp run net - formrecognizer - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| _serviceClient = allOperations; | ||
| _diagnostics = diagnostics; | ||
| _serviceVersion = serviceVersion; | ||
| _operationInternal = new(_diagnostics, this, rawResponse: null, nameof(CreateCustomFormModelOperation)); |
There was a problem hiding this comment.
Since TrainingOperation and CreateComposedModelOperation are subclasses of CreateCustomFormModelOperation, the three of them generate a diagnostic scope with name CreateCustomFormModelOperation.UpdateStatus during update.
This is not the behavior I was expecting, though. I think it would make more sense for each of them to generate an <operation-name>.UpdateStatus scope, according to the name of each class (that's what the OperationInternal class does by default).
We're passing the nameof(CreateCustomFormModelOperation) argument here to set the scope name manually and make sure the original behavior is kept. Should we consider changing it?
There was a problem hiding this comment.
Ohhh this is great! Do you know if this is a breaking change? or if we put a note in our changelog as bug fix will it be ok?
There was a problem hiding this comment.
Strictly speaking, it is a breaking change, but to tell you the truth I'm not familiar with diagnostics so I don't know how users are expected to use this data. We may need to check with architects. I'll create an issue so we don't forget it.
maririos
left a comment
maririos
left a comment
There was a problem hiding this comment.
Just as a safety check, could u run the live tests too?
| _serviceClient = allOperations; | ||
| _diagnostics = diagnostics; | ||
| _serviceVersion = serviceVersion; | ||
| _operationInternal = new(_diagnostics, this, rawResponse: null, nameof(CreateCustomFormModelOperation)); |
There was a problem hiding this comment.
Ohhh this is great! Do you know if this is a breaking change? or if we put a note in our changelog as bug fix will it be ok?
|
/azp run net - formrecognizer - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
All live tests succeeded. |
2 participants
No description provided.