-
Notifications
You must be signed in to change notification settings - Fork 47
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
Fix issue #586 #593
Fix issue #586 #593
Conversation
src/dfmt/ast_info.d
Outdated
+/ | ||
foreach (item; functionCall.arguments.namedArgumentList.items) | ||
{ | ||
// Set to true after first tok!"identifier". |
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 has a bug with e.g. foo([a: b])
inserting that colon, since it just goes over the tokens. Make sure you check if the named argument is even a named argument and not a regular argument, since the grammar for named argument list is:
* $(GRAMMAR $(RULEDEF namedArgument):
* ($(RULE identifer) $(LITERAL ':'))? $(RULE assignExpression)
* ;)
so check for if (item.identifier != tok!"")
before looking for a colon
(I just realized the picked name is not great, but this was just added since the AST closely resembles the grammar which was changed like this in the spec)
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.
Thanks for the quick feedback, I will change the identifier check to if (item.identifier != tok!"")
. The aa example works correctly (I think all nested expressions should), but using the information from libdparse is ofc better.
I didn't know item.identifier
exists, where can I look these things up? So far I went with the files that code-d finds when using "go to definition". The libdparse that I get with dub when building has this class:
final class NamedArgument : BaseNode
{
override void accept(ASTVisitor visitor) const
{
mixin (visitIfNotNull!(name, assignExpression));
}
mixin OpEquals;
/** */ Token name;
/** */ ExpressionNode assignExpression;
/** */ size_t startLocation;
/** */ size_t endLocation;
}
I did find this http://libdparse.dlang.io/grammar.html#namedArgument , where identifier
exists but not for example startLocation
, can I expect that members listed on the website exist additionally to what I can see in the class 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.
I wrote too soon, item.identifier
isn't found, maybe I am using it wrong?
foreach (item; functionCall.arguments.namedArgumentList.items)
{
if (item.identifier == tok!"")
{
continue;
}
// ...
produces
src/dfmt/ast_info.d(472,21): Error: no property `identifier` for `item` of type `const(dparse.ast.NamedArgument)`
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.
ah sorry, it's item.name
, not item.identifier
Yeah the ast.d file contains all the things, and to really understand where they come from in the grammar, you look at parser.d or the auto-generated docs on https://libdparse.dlang.io/grammar.html as well as possibly also the implementation
src/dfmt/ast_info.d
Outdated
|
||
foreach (t; item.tokens) | ||
{ | ||
if (t.type == tok!"comment") |
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.
comment tokens are not part of the tokens
array in the AST, they are only used in trivia tokens (leadingTriviaTokens, trailingTriviaTokens) and are attached to the regular tokens, so you don't have to check for them
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.
Thanks for the info, I will remove the comment check.
Remove custom named arg parser
Feature from this implementation: handles struct initialisers with named arguments the same way as named function arguments (removing this might be difficult, also I think this is probably good for consistency):
The multiline example from issue 586 is rewritten to one line if
--keep_line_breaks=false
which is not an error of the:
formatting and / or expected behaviour (?) since the combined line is still rather short.If
--keep_line_breaks=true
the argument indentation is wrong: but this shouldn't be related to:
formatting and probably requires its own issue (if there isn't one yet):becomes (with
--keep_line_breaks=true
)There are no examples for
f(/+ abc +/ i /+ def +/: 1);
since issue 565 messes with the result (the:
part works as expected but tests will fail if 565 gets fixed).{}
blocks inside the function call due to issues with indentation afterwards (probably related to issue 545).