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

AutoRest should provide a way to mark types and properties as deprecated #1285

Closed
brjohnstmsft opened this issue Jul 18, 2016 · 15 comments · Fixed by Azure/autorest.csharp#58

Comments

@brjohnstmsft
Copy link
Member

As APIs mature, occasionally we decide to remove features. It would be really handy if we could declare our intent to deprecate things in the API spec and have AutoRest flow it through to the generated code. For example, in C# it could add ObsoleteAttribute to generated properties or classes.

brjohnstmsft added a commit to brjohnstmsft/azure-rest-api-specs that referenced this issue Jul 19, 2016
We need to mark a property of this class with the [Obsolete] attribute. We
need to do this by hand until this AutoRest issue is fixed:

Azure/autorest#1285
brjohnstmsft added a commit to brjohnstmsft/azure-rest-api-specs that referenced this issue Jul 22, 2016
We need to mark a property of this class with the [Obsolete] attribute. We
need to do this by hand until this AutoRest issue is fixed:

Azure/autorest#1285
@fearthecowboy
Copy link
Member

I have good news and less good news.

In swagger, there is a deprecated flag http://swagger.io/specification/#operationObject ... but we don't currently do anything with it.

To go along with it, I'd we might think about having a cmdline switch to 'don't generate deprecated APIs'.

This is will fit in nicely in post 1.0 -- opening up the model pipeline will let this work much better.

@tbombach tbombach added this to the 1.0 milestone Aug 8, 2016
brjohnstmsft added a commit to brjohnstmsft/azure-rest-api-specs that referenced this issue Aug 12, 2016
We need to mark a property of this class with the [Obsolete] attribute. We
need to do this by hand until this AutoRest issue is fixed:

Azure/autorest#1285
@brjohnstmsft
Copy link
Member Author

@fearthecowboy This issue isn't fixed. Was it closed by mistake?

@fearthecowboy fearthecowboy reopened this Aug 15, 2016
@fearthecowboy
Copy link
Member

Weird... I never closed it... Did one of your comments in the PR address this?

@brjohnstmsft
Copy link
Member Author

No, but I probably mentioned this issue in a commit comment.

@begoldsm
Copy link
Contributor

@fearthecowboy I like the idea of doing something special with the deprecated flag, but I would caution against simply not generating the APIs, since we need to have deprecated APIs available for a while with the "deprecated warning" for the customer to know that this API will soon disappear. It would be bad if, by flagging something as deprecated, it just disappeared :)

@brjohnstmsft
Copy link
Member Author

+What @begoldsm said. Let SDK authors decide when to remove stuff, not the tool. The point is to have the tool generate code that produces a warning.

@fearthecowboy
Copy link
Member

Hence, my comment:

To go along with it, I'd we might think about having a cmdline switch to 'don't generate deprecated APIs'.

So that the user is in control. Deprecated would be marked by default, optionally not generated by explicit action

@begoldsm
Copy link
Contributor

Ah yes, that makes sense so long as both are available then we are good! I wonder if there is a requirement for the interim step of turning compiler warnings into compiler errors for a deprecated -> officially obsolete -> removed style of timeline. I admit I am not clear on the requirements that we need to adhere to for deprecating and ultimately removing a specific API.

@brjohnstmsft
Copy link
Member Author

@fearthecowboy Ah, that makes more sense now. I'm not sure why we'd need such an option though, as when we fully deprecate something we'd remove it from the next API version and corresponding Open API spec.

@John-Hart John-Hart self-assigned this Aug 23, 2016
John-Hart added a commit that referenced this issue Aug 24, 2016
…property for operations (#1375)

* Added Support for Deprecated property for operations

* Added unit test to verify deprecated operations are attributed correctly
@John-Hart John-Hart removed their assignment Aug 24, 2016
@John-Hart John-Hart removed the C# label Aug 24, 2016
@tbombach tbombach self-assigned this Sep 8, 2016
@annatisch
Copy link
Member

@tbombach, @brjohnstmsft, @John-Hart,
Hi, I was just taking a look at this with regards to the Python generator - it seems that the "deprecated" property only applies to an operation, not (as suggested by the issue title) properties or types - is this correct?
Adding a warning to a deprecated operation in the Python generator should be straight forward, however adding a warning to models or specific properties would be a more complex affair.....

@phil000
Copy link

phil000 commented Oct 24, 2016

@tbombach, @brjohnstmsft, @John-Hart

I've been having a look at this code-gen in AutoRest.0.17.1-Nightly20161023.nupkg for the use of Obsolete in C# generated client.

There are 3 ways to invoke the operation through the generated client but only 1 of them gets the obsolete attribute and generates a compiler warning:

var response1 = client.Permissions.CreatePermission(request); OK, gets warning
var response2 = client.Permissions.CreatePermissionAsync(request).Result; NO WARNING
var response3 = client.Permissions.CreatePermissionWithHttpMessagesAsync(request).Result; NO WARNING

@brjohnstmsft
Copy link
Member Author

brjohnstmsft commented Feb 25, 2017

@olydis Looks like you've taken over this issue. Do you have any thoughts on supporting something like Swagger's "deprecated" setting for properties and types in addition to operations?

@olydis olydis added the C# label Feb 28, 2017
@olydis
Copy link
Contributor

olydis commented Feb 28, 2017

Ah yes, I'd have no problem doing that for the C# generator, but there are currently more pressing TODOs we gotta work on :-|

@lmazuel
Copy link
Member

lmazuel commented Apr 12, 2017

Made the Python spec here: #2096

@fearthecowboy fearthecowboy removed this from the 1.0 milestone Jun 22, 2017
@fearthecowboy
Copy link
Member

Adding the notion of supporting 'obsolete' and 'deprecated' to vNext generators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants