-
Notifications
You must be signed in to change notification settings - Fork 72
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
Build a detailed definition for an Autocomplete Response #661
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs tests
DSymbol* sym = cast(DSymbol*) symbol; | ||
DSymbol* symType = sym.type; | ||
|
||
while (symType && sym && sym.type != sym) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should have a counter / max limit just in case there are infinite cycles to break after e.g. 50 iterations or so
{ | ||
// if the symbol has a calltip, then let's use that | ||
// otherwise build something with its type | ||
if (sym.callTip.length > 0 && indexOf(sym.callTip, " ") != -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (sym.callTip.length > 0 && indexOf(sym.callTip, " ") != -1) | |
if (sym.callTip.length > 0 && sym.callTip.canFind(" ")) |
canFind is more readable than indexOf() == -1
{ | ||
definition = sym.callTip; | ||
} | ||
else if (sym.type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else if (sym.type) | |
else if (symType) |
string symName = symbol.name; | ||
string symTypeName = symType.callTip.length > 0 ? symType.callTip : symType.name; | ||
definition = symTypeName ~ " " ~ symName; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you seem to be discarding sym.callTip here if it has content, but no whitespace
|
||
// if that's a function, and definition doesn't have white space | ||
// that mean the function returns either auto/enum | ||
if (sym.kind == CompletionKind.functionName && indexOf(definition, " ") == -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (sym.kind == CompletionKind.functionName && indexOf(definition, " ") == -1) | |
if (sym.kind == CompletionKind.functionName && !definition.canFind(" ")) |
// if that's a function, and definition doesn't have white space | ||
// that mean the function returns either auto/enum | ||
if (sym.kind == CompletionKind.functionName && indexOf(definition, " ") == -1) | ||
{ | ||
definition = "auto " ~ definition; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this whole check might be trying to be too smart, just checking for a space is bound to have issues further down the line e.g. with templates/template arguments that have spaces in them.
I would suggest removing this special case for now to keep the diff smaller, this looks to me rather like something that needs to be changed on dsymbol to be more effective.
This needs: dlang-community/dsymbol#175 to be merged Then DCD will need to pick up a version of dsymbol with that PR merged Once this is merged, we'll need to merge #660
This mostly resolve cases where auto/enum/alias is used, it'll try to resolve the type and build a proper definition for it
This needs: dlang-community/dsymbol#175 to be merged
Then DCD will need to pick up a version of dsymbol with that PR merged
Once this is merged, we'll need to merge #660
Once everything done, we'll need to reflect the changes in serve-d