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.

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

Add --fix-typedefs to Dart formatter. #743

Merged
merged 8 commits into from Nov 8, 2018
Merged

Conversation

lrhn
Copy link
Member

@lrhn lrhn commented Sep 19, 2018

WDYT?
Would it be better to build a new node for the converted syntax and then visit that node, instead of physically building the output? It would ensure consistent formatting.

Copy link
Member

@munificent munificent left a comment

Choose a reason for hiding this comment

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

Thank you for putting time into this! It will be a really nice fix to have.

(As is usual with dart_style), I'd like there to be less duplication in the visitor and more scrupulous tests before this lands.

lib/src/source_visitor.dart Show resolved Hide resolved
// Old style function parameters or single-identifier-meaning-name
// cannot occur inside the `Function` type expression.
// If they do anyway, retain them as an error.
bool wasInsideNewTypedefFix = _insideNewTypedefFix;
Copy link
Member

Choose a reason for hiding this comment

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

bool -> var

Is there a reason you don't just go ahead and fix them?

@@ -1836,11 +1911,16 @@ class SourceVisitor extends ThrowingAstVisitor {
modifier(node.keyword);

visit(node.type);
bool hasType = node.type != null;
if (_insideNewTypedefFix && !hasType) {
_writeText("dynamic", node.identifier.offset);
Copy link
Member

Choose a reason for hiding this comment

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

This needs a test for what happens to a comment before the identifier.

Copy link
Member Author

@lrhn lrhn Oct 1, 2018

Choose a reason for hiding this comment

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

Ick, good point.
I assume comments are attached to the following token, and that we want comments before the lone identifier to go before the inserted dynamic.

I'll make that so.

lib/src/style_fix.dart Outdated Show resolved Hide resolved
test/fixes/typedefs.unit Show resolved Hide resolved
test/fixes/typedefs.unit Show resolved Hide resolved
@lrhn
Copy link
Member Author

lrhn commented Sep 28, 2018

Didn't address all comments yet, but refactored to avoid code duplication.

// without a name. Add "dynamic" in that case.

// Ensure comments on the identifier comes before the inserted type.
token(node.identifier.token, before: () {
Copy link
Member Author

Choose a reason for hiding this comment

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

Here I am kind-of inlining visitSimpleIdentifier, which just calls token(node.token).
Should I instead add an {void before()} or {void beforeToken} parameter to visitSimpleIdentifier and call that directly?

I do want to insert the dynamic between the comment and the token itself, which token(..., before: ...) does for me, but I can't put that extra aargument through the visit/accept procedure, and I don't want to store it in some object-global state.

@lrhn
Copy link
Member Author

lrhn commented Oct 12, 2018

All comments addressed. PTAL

@munificent munificent merged commit 06c3636 into dart-lang:master Nov 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants