Cleanup Deprecated Metadata #2129
Conversation
| bool | ||
| Slice::Contained::isDeprecated(bool checkParent) const | ||
| { | ||
| const string deprecate = "deprecate"; |
There was a problem hiding this comment.
Even though your functions are named "isDeprecated", you didn't add "deprecated" as an alias for the "deprecate" metadata?
There was a problem hiding this comment.
Yes, but only because isDeprecate isn't a sensible thing to say IMO. Nothing to do with our eventual renaming.
I plan to add the alias in my next PR, alongside removing the dispatch side.
cpp/src/slice2cpp/Gen.cpp
Outdated
| string deprecateMetadata, deprecateSymbol; | ||
| if (p1->findMetaData("deprecate", deprecateMetadata) || | ||
| (p2 != 0 && p2->findMetaData("deprecate", deprecateMetadata))) | ||
| string deprecateSymbol; |
There was a problem hiding this comment.
I'd use "deprecated" instead of "deprecate".
There was a problem hiding this comment.
Sure. deprecateSymbol was just the old name.
| [["swift:class-resolver-prefix:IceDefaultValue", | ||
| "suppress-warning:deprecated"]] // For enumerator references | ||
| [["swift:class-resolver-prefix:IceDefaultValue"]] | ||
| [["suppress-warning:deprecated"]] // For enumerator references |
There was a problem hiding this comment.
For suppress warning, we already use deprecated?
There was a problem hiding this comment.
Yep. Of course! See: #2093 (comment)
And maybe weigh in on the rest of that comment.
| bool | ||
| Slice::Contained::isDeprecated(bool checkParent) const | ||
| { | ||
| const string deprecate = "deprecate"; |
There was a problem hiding this comment.
I would be nice to update findMetaData to take a string_view and switch this const to a constexpr.
There was a problem hiding this comment.
Sure, I'll do that in a follow-up PR though.
I want to avoid going too far down that rabbit hole though.
It'd probably take months to clean and modernize the compilers...
cpp/src/slice2cpp/Gen.cpp
Outdated
| { | ||
| string msg = "is deprecated"; | ||
| if (deprecateMetadata.find("deprecate:") == 0 && deprecateMetadata.size() > 10) | ||
| if (auto reason = p1->getDeprecationReason(false)) |
There was a problem hiding this comment.
Should this be true as well? Since we're checking the parent above?
There was a problem hiding this comment.
Yes, you're right. Fixed.
| { | ||
| _out << nl << " * @deprecated " << deprecateReason; | ||
| _out << nl << " * @deprecated"; | ||
| if (auto reason = p->getDeprecationReason(false)) |
There was a problem hiding this comment.
My guess is that if a parent is deprecated, we only want to generate a comment once (on that parent),
instead of generating it on everything inside of it?
This is just what we were already doing though. If you look at the code underneath this:
writeDocComment(p, getDeprecateReason(p, 0, "type"));
The 0 means: don't bother with the parent.
And we always pass 0 for javascript.
cpp/src/slice2cs/Gen.cpp
Outdated
| @@ -83,28 +83,35 @@ namespace | |||
| return "???"; | |||
| } | |||
|
|
|||
| string getDeprecateReason(const ContainedPtr& p1, const ContainedPtr& p2, const string& type) | |||
| string getDeprecateReason(const ContainedPtr& p1, const string& type) | |||
There was a problem hiding this comment.
I don't know if we should generate a special doc-comment message for deprecated operation and the like.
But we should at least give this helper function a clearer/consistent name, like getDocCommentDeprecationReason.
| @@ -1625,15 +1618,10 @@ Slice::JavaVisitor::writeDispatch(Output& out, const InterfaceDefPtr& p) | |||
|
|
|||
| out << sp; | |||
| writeHiddenDocComment(out); | |||
| for (OperationList::iterator r = allOps.begin(); r != allOps.end(); ++r) | |||
| for (const OperationPtr& op : allOps) | |||
There was a problem hiding this comment.
I would add a TODO: temporary.
This PR addresses part of #2085.
Now, when the Slice compiler generates a
[[deprecated]],[Obsolete], etc. something to mark an item as deprecated,it will only supply a message when the user did.
Whereas right now, if the user didn't supply a message, we auto-compute one.
#2085 points out that this auto-computed message is broken anyways.
While fixing this, I also did some cleanup. Mostly adding 2 functions to the
Parser:isDeprecatedandgetDeprecateReason.Instead of our previous approach of defining approximate copies of these functions in each
slice2xxxseparate folder...After this PR I'll address the 2nd part of #2085, which is removing the 'deprecated' metadata from the dispatch side of things.