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.

Sign up for GitHub

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

Improve JavaScript doc comments for generated code #2604

Merged
merged 2 commits into from
Aug 2, 2024
Merged

Improve JavaScript doc comments for generated code #2604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a6399c
JavaScript doc comment fixes
pepone Aug 2, 2024
efae928
JavaScript doc comment improvements
pepone Aug 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions cpp/src/Slice/Parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,6 +732,12 @@ namespace

CommentPtr
Slice::Contained::parseComment(bool stripMarkup) const
{
return parseComment(_comment, stripMarkup);
}

CommentPtr
Slice::Contained::parseComment(const string& text, bool stripMarkup) const
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this overload to be able to preprocess the coment before parsing. This allow for language specific preprocessing.

{
CommentPtr comment = make_shared<Comment>();

Expand All @@ -745,15 +751,15 @@ Slice::Contained::parseComment(bool stripMarkup) const
comment->_deprecated.push_back(IceInternal::trim(*reason));
}

if (!comment->_isDeprecated && _comment.empty())
if (!comment->_isDeprecated && text.empty())
{
return nullptr;
}

//
// Split up the comment into lines.
//
StringList lines = splitComment(_comment, stripMarkup);
StringList lines = splitComment(text, stripMarkup);

StringList::const_iterator i;
for (i = lines.begin(); i != lines.end(); ++i)
Expand Down
1 change: 1 addition & 0 deletions cpp/src/Slice/Parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ namespace Slice
std::string file() const;
std::string line() const;
std::string comment() const;
CommentPtr parseComment(const std::string&, bool) const;
CommentPtr parseComment(bool) const;

int includeLevel() const;
Expand Down
64 changes: 43 additions & 21 deletions cpp/src/slice2js/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,38 @@ using namespace IceInternal;

namespace
{
CommentPtr parseComment(const ContainedPtr& p)
{
// JavaScript TypeDoc doc processor doesn't accept # at the beginning of a link
// so we need to remove it.
Comment on lines +27 to +28
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these generate valid links still?

Copy link
Member Author

@pepone pepone Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, with Slice and Java doc you can use {@link #foo} to refer to a member of the class. With TypeDoc is just {@link foo}

This was causing warnings mentioned above when parasing

ice/slice/Ice/Metrics.ice

Lines 134 to 136 in 5d4f24b

/// The number of threads which are currently performing other activities. These are all other that are not
/// counted with {@link #inUseForUser} or {@link #inUseForIO}, such as DNS lookups, garbage collection).
int inUseForOther = 0;

string text = p->comment();
const string linkBegin = "{@link ";
const string linkEnd = "}";

string::size_type pos = text.find(linkBegin);
while (pos != string::npos)
{
string::size_type endPos = text.find(linkEnd, pos);
if (endPos == string::npos)
{
// Invalid link, ignore it
break;
}

string link = text.substr(pos + linkBegin.size(), endPos - pos - linkBegin.size());
if (link.find("#") == 0)
{
link = link.substr(1);
}
const string replacement = "{@link " + link + "}";

text.replace(pos, endPos - pos + linkEnd.size(), replacement);
pos = text.find(linkBegin, pos + replacement.size());
}

return p->parseComment(text, false);
}

// Convert a path to a module name, e.g., "../foo/bar/baz.ice" -> "__foo_bar_baz"
string pathToModule(const string& path)
{
Expand Down Expand Up @@ -206,7 +238,7 @@ namespace
return;
}

CommentPtr doc = p->parseComment(false);
CommentPtr doc = parseComment(p);

out << nl << "/**";

Expand Down Expand Up @@ -252,7 +284,6 @@ namespace
const OperationPtr& op,
const CommentPtr& doc,
OpDocParamType type,
const StringList& preParams = StringList(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this param, we were always passing an empty list.

const StringList& postParams = StringList())
{
ParamDeclList params;
Expand All @@ -269,11 +300,6 @@ namespace
break;
}

if (!preParams.empty())
{
writeDocLines(out, preParams, true);
}

map<string, StringList> paramDoc = doc->parameters();
for (ParamDeclList::iterator p = params.begin(); p != params.end(); ++p)
{
Expand Down Expand Up @@ -303,9 +329,10 @@ namespace
ExceptionPtr ex = op->container()->lookupException(name, false);
if (ex)
{
name = ex->scoped().substr(2);
name = ex->scoped();
}
out << nl << " * @throws " << name << " ";
name = JsGenerator::fixId(name);
out << nl << " * @throws {@link " << name << "} ";
writeDocLines(out, p->second, false);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeDoc uses this syntax in throws see: https://typedoc.org/tags/throws/

}
}
Expand All @@ -315,8 +342,6 @@ namespace
const OperationPtr& op,
const CommentPtr& doc,
OpDocParamType type,
bool showExceptions,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, we should always write the @throws clauses. We were passing "false" all the time and exceptions were ignored.

const StringList& preParams = StringList(),
const StringList& postParams = StringList(),
const StringList& returns = StringList())
{
Expand All @@ -327,18 +352,15 @@ namespace
writeDocLines(out, doc->overview(), true);
}

writeOpDocParams(out, op, doc, type, preParams, postParams);
writeOpDocParams(out, op, doc, type, postParams);

if (!returns.empty())
{
out << nl << " * @return ";
writeDocLines(out, returns, false);
}

if (showExceptions)
{
writeOpDocExceptions(out, op, doc);
}
writeOpDocExceptions(out, op, doc);

if (!doc->misc().empty())
{
Expand Down Expand Up @@ -2519,7 +2541,7 @@ Slice::Gen::TypeScriptVisitor::visitClassDefStart(const ClassDefPtr& p)
_out << nl << " * One-shot constructor to initialize all data members.";
for (const auto& dataMember : allDataMembers)
{
CommentPtr comment = dataMember->parseComment(false);
CommentPtr comment = parseComment(dataMember);
if (comment)
{
_out << nl << " * @param " << fixId(dataMember->name()) << " " << getDocSentence(comment->overview());
Expand Down Expand Up @@ -2612,7 +2634,7 @@ Slice::Gen::TypeScriptVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
}

const string contextParam = escapeParam(paramList, "context");
CommentPtr comment = op->parseComment(false);
CommentPtr comment = parseComment(op);
const string contextDoc = "@param " + contextParam + " The Context map to send with the invocation.";
const string asyncDoc = "The asynchronous result object for the invocation.";

Expand All @@ -2622,7 +2644,7 @@ Slice::Gen::TypeScriptVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
StringList postParams, returns;
postParams.push_back(contextDoc);
returns.push_back(asyncDoc);
writeOpDocSummary(_out, op, comment, OpDocInParams, false, StringList(), postParams, returns);
writeOpDocSummary(_out, op, comment, OpDocInParams, postParams, returns);
}
_out << nl << fixId(op->name()) << spar;
for (const auto& param : inParams)
Expand Down Expand Up @@ -2715,7 +2737,7 @@ Slice::Gen::TypeScriptVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
}

const string currentParam = escapeParam(inParams, "current");
CommentPtr comment = p->parseComment(false);
CommentPtr comment = parseComment(op);
const string currentDoc = "@param " + currentParam + " The Current object for the invocation.";
const string resultDoc = "The result or a promise like object that will "
"be resolved with the result of the invocation.";
Expand All @@ -2724,7 +2746,7 @@ Slice::Gen::TypeScriptVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
StringList postParams, returns;
postParams.push_back(currentDoc);
returns.push_back(resultDoc);
writeOpDocSummary(_out, op, comment, OpDocInParams, false, StringList(), postParams, returns);
writeOpDocSummary(_out, op, comment, OpDocInParams, postParams, returns);
}
_out << nl << "abstract " << fixId(op->name()) << spar;
for (const auto& param : inParams)
Expand Down
34 changes: 20 additions & 14 deletions js/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions js/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,18 @@
"types": "src/index.d.ts",
"browserslist": "> 0.25%, not dead",
"devDependencies": {
"@types/node": "^20.14.2",
"@types/node": "^22.1.0",
"del": "^7.1.0",
"gulp": "^5.0.0",
"gulp-ext-replace": "^0.3.0",
"gulp-ice-builder": "^3.0.4",
"gulp-typescript": "^5.0.1",
"http-server": "^14.1.1",
"jshint": "^2.13.6",
"prettier": "^3.3.2",
"prettier": "^3.3.3",
"pump": "^3.0.0",
"typedoc": "^0.26.3",
"typescript": "^5.5.3",
"typedoc": "^0.26.5",
"typescript": "^5.5.4",
"typescript-formatter": "^7.2.2",
"vinyl": "^3.0.0",
"vinyl-paths": "^5.0.0"
Expand Down
4 changes: 2 additions & 2 deletions js/src/Ice/Connection.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,8 @@ declare module "ice" {
/**
* Throw an exception indicating the reason for connection closure. For example,
* {@link CloseConnectionException} is raised if the connection was closed gracefully, whereas
* {@link ConnectionManuallyClosedException} is raised if the connection was manually closed by
* the application. This operation does nothing if the connection is not yet closed.
* {@link ConnectionAbortedException}/{@link ConnectionClosedException} is raised if the connection was
* manually closed by the application. This operation does nothing if the connection is not yet closed.
*/
throwException(): void;
}
Expand Down
13 changes: 10 additions & 3 deletions js/src/Ice/LocalExceptions.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,8 @@ declare module "ice" {
*/
class UnknownException extends LocalException {
/**
* One-shot constructor to initialize all data members.
* @param unknown This field is set to the textual representation of the unknown exception if available.
* @param ice_cause The error that cause this exception.
* Constructs an unknown exception.
* @param message The exception message.
*/
constructor(message: string);
unknown: string;
Expand All @@ -53,13 +52,21 @@ declare module "ice" {
* The dispatch failed with a {@link LocalException} that is not one of the special marshal-able local exceptions.
*/
class UnknownLocalException extends UnknownException {
/**
* Constructs an unknown local exception.
* @param message The exception message.
*/
constructor(message: string);
}

/**
* The dispatch returned a {@link UserException} that was not declared in the operation's exception specification.
*/
class UnknownUserException extends UnknownException {
/**
* Constructs an unknown user exception.
* @param message The exception message.
*/
constructor(message: string);
}

Expand Down
2 changes: 1 addition & 1 deletion js/src/Ice/ObjectAdapter.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ declare module "ice" {
/**
* Install a middleware in this object adapter.
*
* @param The middleware to install.
* @param middleware The middleware to install.
* @return This object adapter.
* @throws Error Thrown if the object adapter's dispatch pipeline has already been
* created. This creation typically occurs the first time the object adapter dispatches an incoming request.
Expand Down
Loading