Conversation
There was a problem hiding this comment.
You may want to use x-ms-flatten for CreateJobProperties type to improve user experience.
There was a problem hiding this comment.
Yes, this would be ideal. However, I'm not sure if it's appropriate for this situation because when a user is creating a job, we want them to be explicit on which properties object to use: CreateScopeJobProperties vs CreateUSqlJobProperties. That is, since CreateJobProperties is polymorphic via the discriminator value, won't this conflict with the flatten feature?
There was a problem hiding this comment.
I didn't see that - thanks for explaining!
There was a problem hiding this comment.
Consider using x-ms-flatten. Unless it'll introduce a breaking change.
There was a problem hiding this comment.
This is related to the "CreateJobProperties" topic above in that JobProperties is polymorphic via the discriminator value. But JobInformation is primarily used for GETs now. So, I would be in favor of flattening this. However, it would be a breaking change. @matt1883, thoughts?
There was a problem hiding this comment.
Here is an example of the benefits: https://github.com/Azure/autorest/blob/master/docs/extensions/readme.md#x-ms-client-flatten
There was a problem hiding this comment.
I would like to do this, but I confirmed with @olydis that not every language supports x-ms-flatten for property objects that are polymorphic via the discriminator value. We will definitely consider this an upcoming release. Thank you for mentioning this!
There was a problem hiding this comment.
Renaming is a breaking change - please be aware of that. Also you'll need to update PowerShell & CLI cmdlets if you have any.
There was a problem hiding this comment.
Thank you for noting this. I should have been clear in my notes, but this is actually a bugfix. Yes it would be a breaking change, but the service returns this as TotalPausedTime. I don't expect this property to be too popular among users. Will keep an eye out for this when I update PS and CLI.
There was a problem hiding this comment.
Consider true if the list can grow.
There was a problem hiding this comment.
Yes, this is the plan for a release in Oct. I am waiting for the C# team to implement an extensible enum feature. If I mark this as "true" now, it'll be a big breaking change for C# users.
There was a problem hiding this comment.
The documentation of model properties MUST NOT start with the phrase "Gets or sets ..", "Gets ..", "Sets .."
There was a problem hiding this comment.
Is this referring to line 1971?
There was a problem hiding this comment.
Yes - sorry I don't know why it ends up here
There was a problem hiding this comment.
Fixed! I got rid of "Gets"
|
Hi There, I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result: File: File: File: 💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡 AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback Thanks for your co-operation. |
…reateScopeJobProperties
|
I pushed one more simple commit -- adding a "notifier" field to ScopeJobProperties and CreateScopeJobProperties. |
|
Hi There, I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result: File: File: File: 💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡 AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback Thanks for your co-operation. |
|
No modification for AutorestCI/azure-sdk-for-node |
7 participants
The substantial changes (listed below) are in this file: specification/datalake-analytics/data-plane/Microsoft.DataLakeAnalytics/2017-09-01-preview/job.json
Created a new API version, 2017-09-01-preview, for job.json
Added a tags field to the JobInformationBasic object
Created the following objects:
Added two new APIs: UpdateJob and YieldJob
Added more properties to the JobStatisticsVertexStage object
A minor bug fix for the files below: adding two missing enum properties "Canceled" and "Undeleting" for the provisioningState field.
This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
PR information
api-versionin the path should match theapi-versionin the spec).Quality of Swagger