Skip to content

Allow additional field deprecatedMessage for operation #540

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

Closed
lieldulev opened this issue Jan 15, 2016 · 20 comments
Closed

Allow additional field deprecatedMessage for operation #540

lieldulev opened this issue Jan 15, 2016 · 20 comments

Comments

@lieldulev
Copy link

field name deprecatedMessage
type string.
optional.
default "".
Meaningful only if depreacted is true.

Usually deprecating a command/function/endpoint comes with a message, helping user to migrate to alternative endpoint/api/service.

https://en.wikipedia.org/wiki/Deprecation#Software_deprecation

@whitlockjc
Copy link
Member

For simplicity, why not allow the deprecated property take a string? Then we don't have to be so verbose.

@lieldulev
Copy link
Author

Are you suggesting deprecated to take either boolean or string?
Or just change it to string and have empty string ( "" ) as the default value and mean not deprecated?

@whitlockjc
Copy link
Member

Yes, it takes a boolean or a string. The idea is for simple cases, turning it on (deprecated: true) would be enough. But for the case you want to allow, it could be done with the same property (deprecated: 'This field has been replaced by that field'). Heck, you might even allow GFM in the value to allow for styled rendering of the deprecation message.

Processing rules:

  • Property not set: Not deprecated
  • Property is false: Not deprecated
  • Propery is true: Deprecated
  • Property is a string: Deprecated

Just an idea...

@IvanGoncharov
Copy link
Contributor

@whitlockjc @lieldulev I think we already have two nice fields for that title could be [Deprecated] ... and description could be **Deprecated**: ....
So it's just matter of including both fields in warning output.

@whitlockjc
Copy link
Member

Well, I figure this proposal is more for a general property and not just for the overall API itself. Imagine being able to mark operations, paths, parameters, ... as deprecated as your API evolves.

@IvanGoncharov
Copy link
Contributor

IMHO, I think Swagger should have minimal amount features which allow to describe API.
So if you want deprecate path, just mark every operation inside it as deprecated(usual it 2-3 operations).

Deprecation attribute for parameter can be useful, but parameters also have description attribute.
So solution is the same as for operations, just include description into warning message.

@whitlockjc
Copy link
Member

The issue with relying on description is now you have to parse it for special tokens and that is just more work. (It's work all around, I know.) We could easily get away with this using a vendor extension but to me, deprecating various parts of your API design is something that is globally useful which makes it a good candidate for core specification support.

@IvanGoncharov
Copy link
Contributor

Why you need to parse anything?
If it client side tool like swagger-js just include full description into warning.
Something like:

POST for '/foo/bar' was deprecated, please read description for this method:
<description from Swagger>

If it some docs, you need deprecation notice in description independently of deprecated field.
It's even more reliable because you want it to be displayed everywhere, e.g. Postman, SoapUI

So no need for special tokens just write deprecation notice as part of documenting method, parameter, ...

@IvanGoncharov
Copy link
Contributor

I used ***Deprecated*** as GFM, so no special meaning intended.
Same goes for [Deprecated] I just use brackets to separate it from rest of the text.

@whitlockjc
Copy link
Member

Marking something as deprecated is more than just a display issue. Sure, if I was just rendering a Markdown view I could just throw in some special text and things would look nice. But really, marking something as deprecated is a piece of state, something that other tooling might want to use. For that case, you do have to parse things to identify the deprecated components.

@whitlockjc
Copy link
Member

Imagine I want to create an object model around the specification. I wanted to have a deprecated property or #isDeprecated() method on my objects. (See the object model I've created in Sway: https://github.com/apigee-127/sway/blob/master/docs/API.md) I want to use this for display purpose of course but also maybe logging and potentially other things. Creating this model would be much simpler if marking things as deprecated was a first class citizen and not some text-based convention that makes processing things more painful. That's all I'm trying to say.

@IvanGoncharov
Copy link
Contributor

But really, marking something as deprecated is a piece of state, something that other tooling might want to use.

Yes exactly, that is why deprecated property with boolean value exist.
This issue about adding human-readable text to deprecated state and where to put it.
I think it belongs to description field, same as the rest of human-readable text.

@lieldulev
Copy link
Author

@IvanGoncharov I suggested a special field because I found current description/title not enough.

I have 2 main reasons:

Specifically - in our swagger-ui - I'd like to present the deprecation message in a different way than endpoint description, currently I have to "dirty" my description with html tags to do that.

I also don't like the fact that a method "usage/description" changes because of deprecation. Deprecation doesn't change how to use an endpoint/parameter or what it does (or supposed to do), it (usually) means that the thing will be removed in the near future - and suggest that the developer will move away from it while pointing me to an alternative solution.

@IvanGoncharov
Copy link
Contributor

I also don't like the fact that a method "usage/description" changes because of deprecation.

description is purely human readable field and should be treated as such.
So it can change when you fix spelling mistakes, adding formatting, etc.

One of example of how it could be represented in docs is Stripe API:
https://stripe.com/docs/api#recipients
Deprecated badge can be added based on boolean value and migration guide is part of description.
I'm not saying it is only possible way to document it, but it looks good and serve it purpose.

Could it be better represented with separate field? Probably yes.
Could we meet industrial standard without separate field? Also yes.

@whitlockjc
Copy link
Member

My stance has been based on the original request to add another field for the deprecation message. I'd rather not add another field so that's why I suggested extending the deprecated field to also allow a string value which would turn deprecation on with a message. As long as you can put deprecated properties in all the places we need and all of those same places allow a free-form description property, I don't see the need of a new property or changing the existing deprecated property. As a tooling author, I just don't want to have to parse the description property to identify if something is deprecated.

@IvanGoncharov
Copy link
Contributor

As long as you can put deprecated properties in all the places we need and all of those same places allow a free-form description property, I don't see the need of a new property or changing the existing deprecated property.

I fully agree 👍
One thing that could be added to Swagger spec, is recommendation to add full content of description field into text of deprecation warning.

@mkrufky
Copy link

mkrufky commented Jan 15, 2016

+1 on @whitlockjc 's revision - extend the deprecated field to be of type boolean or string to satisfy @lieldulev 's missing feature.

@webron
Copy link
Member

webron commented Jan 19, 2016

I tend to believe the structure should stay as-is now. @lieldulev - if you change your documentation marking that a method is deprecated, then yes, it would make perfect sense to change its description as well. You can say that it doesn't change how the operation is to be used, but that's not entirely true. You're pretty much redirecting the user to a different operation, or give them an entirely different description of what they should be doing.

Can't say that swagger-ui is a reason for this change as well. You have the information that the method is deprecated and can present it as such. If you must do some additional display magic to present the additional details in a different manner, you can even do it in the current implementation, all with some html/css/javascript magic. It may not be pretty, but it is doable.

Lastly, while @whitlockjc's suggestion is relatively elegant, I'm not a big fan of type overloading and for that reason I'm not in favor of this solution.

@lieldulev
Copy link
Author

After reading your arguments and thinking more about it, I changed my mind.

Although I still believe we should be able to define more data regarding deprecation (e.g. version, date), I actually don't think bloating the spec with a very custom use case by adding deprecatedMessage is the answer, and I'm also not a fan of type overloading.

Unless anyone has a more elegant solution than what I do today (custom HTML inside the endpoint's description) - for now let's close this issue.

Thank you for your remarks.

@webron
Copy link
Member

webron commented Jan 27, 2016

Thanks for the feedback, @lieldulev. I'd suggest we close it for now, and revisit if needed.

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

No branches or pull requests

5 participants