Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17470004:
Dart formatter checkpoint. (Closed)
|
Created:
7 years, 6 months ago by pquitslund Modified:
7 years, 6 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionDart formatter checkpoint.
More babysteps. Note: some of the style is tentative (e.g., still trying on cascades).
Comments welcome.
R=brianwilkerson@google.com, scheglov@google.com
Committed: https://code.google.com/p/dart/source/detail?r=24255
Patch Set 1 #
Total comments: 29
Messages
Total messages: 5 (0 generated)
LGTM https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... File pkg/analyzer_experimental/lib/src/services/formatter_impl.dart (right): https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:155: EditRecorder(this.options): editStore = new EditStore(); Just curiosity, but why did you move away from using a field formal parameter in CodeFormatterImpl, but not here? And how did you decide which of these possibilities to use: EditRecorder(this.options): editStore = new EditStore(); EditRecorder(FormatterOptions options): this.options = options, editStore = new EditStore(); EditRecorder(FormatterOptions options): this(options, new EditStore()); or any variation that uses a block body, or combining this with the 'forStore' constructor into a single EditRecorder(this.options, [this.editStore = new EditStore()]); https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:301: ++charsToReplace; Does this want to be incremented by NEW_LINE.length? https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:375: const EditStore(); I wouldn't make this a 'const' constructor because edit stores are mutable, but perhaps I'm thinking of 'const' the wrong way. https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:477: ..currentToken = start; This looks weird. I would have expected the state of the recorder to have been configured before passing it to this method and for the 'source' and 'start' arguments to be removed. (The reference to 'source' below would presumably be replaced by 'recorder.source'.) https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:507: You missed the withClause. https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:517: recorder.newline(); Do you want to advanceIndent() after the newline, or are you expecting every member to do that before starting to write themselves out? https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:529: recorder..advance(node.beginToken) The begin token for a BlockFunctionBody is the '{' for the block. I don't think you want to advance over it here, but leave it for the block to do. (I don't think you ever want to use 'beginToken' or 'endToken'; either there are more specific getters or they belong to a child.) https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:545: recorder..advance(node.beginToken) Here too: use 'functionDefinition' and 'semicolon'. https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:557: recorder.advanceIndent(); The rule for AST nodes is that whitespace before and after the node belongs to the parent. I think you want to follow something similar here: the whitespace before and after a node should be written while visiting the parent of the node. I think it will lead to fewer mistakes, because you'll have more context when deciding what whitespace to write. https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:565: recorder.advance(node.returnType.beginToken); I think you want to visit the returnType at this point.
lgtm https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... File pkg/analyzer_experimental/lib/src/services/formatter_impl.dart (right): https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:86: final List<AnalysisError> errors = <AnalysisError>[]; Remove type annotation for the final field. https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:89: CodeFormatterImpl(FormatterOptions options) : this.options = options, May be "this.options" and remove initializer? https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:232: for (var i=0; i < indentWidth; ++i) { Whitespaces before/after '='. https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/tes... File pkg/analyzer_experimental/test/services/formatter_test.dart (right): https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/tes... pkg/analyzer_experimental/test/services/formatter_test.dart:51: There was no empty line in the same code in test before. Do we need it? https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/tes... pkg/analyzer_experimental/test/services/formatter_test.dart:111: expect(doFormat(recorder).length, equals((src+ NEW_LINE).length)); Space before '+'.
Thanks for the thoughtful comments. Much appreciated! https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... File pkg/analyzer_experimental/lib/src/services/formatter_impl.dart (right): https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:86: final List<AnalysisError> errors = <AnalysisError>[]; On 2013/06/19 22:12:05, scheglov wrote: > Remove type annotation for the final field. Done. https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:89: CodeFormatterImpl(FormatterOptions options) : this.options = options, On 2013/06/19 22:12:05, scheglov wrote: > May be "this.options" and remove initializer? The trouble is that I'd like to use options in my creation of the EditRecorder AND I'd like recorder to be final. Unfortunately, I can't access options in the initializer block and that's the only place I can assign to final fields... (Sorry if this is confusing. Feel free to walk over if you want me to show you what's up.) https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:155: EditRecorder(this.options): editStore = new EditStore(); Discussed (exhaustively) in person! https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:232: for (var i=0; i < indentWidth; ++i) { On 2013/06/19 22:12:05, scheglov wrote: > Whitespaces before/after '='. Done. https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:301: ++charsToReplace; On 2013/06/19 21:50:52, Brian Wilkerson wrote: > Does this want to be incremented by NEW_LINE.length? Yes! Good catch. :) https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:375: const EditStore(); On 2013/06/19 21:50:52, Brian Wilkerson wrote: > I wouldn't make this a 'const' constructor because edit stores are mutable, but > perhaps I'm thinking of 'const' the wrong way. Actually, I think I AM! Honestly, this is an artifact of an attempt to make this constructor callable from with a constructor initializer block. Fixed now. https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:477: ..currentToken = start; On 2013/06/19 21:50:52, Brian Wilkerson wrote: > This looks weird. I would have expected the state of the recorder to have been > configured before passing it to this method and for the 'source' and 'start' > arguments to be removed. (The reference to 'source' below would presumably be > replaced by 'recorder.source'.) Agreed. Still working on how this entry point should look. Thanks! https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:507: On 2013/06/19 21:50:52, Brian Wilkerson wrote: > You missed the withClause. Coming real soon. (Driven by accompanying tests.) https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:517: recorder.newline(); On 2013/06/19 21:50:52, Brian Wilkerson wrote: > Do you want to advanceIndent() after the newline, Actually, yes. That is cleaner. Thanks! https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:545: recorder..advance(node.beginToken) On 2013/06/19 21:50:52, Brian Wilkerson wrote: > Here too: use 'functionDefinition' and 'semicolon'. Thanks! https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:557: recorder.advanceIndent(); On 2013/06/19 21:50:52, Brian Wilkerson wrote: > The rule for AST nodes is that whitespace before and after the node belongs to > the parent. I think you want to follow something similar here: the whitespace > before and after a node should be written while visiting the parent of the node. > I think it will lead to fewer mistakes, because you'll have more context when > deciding what whitespace to write. Done. https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:565: recorder.advance(node.returnType.beginToken); On 2013/06/19 21:50:52, Brian Wilkerson wrote: > I think you want to visit the returnType at this point. Done. https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/tes... File pkg/analyzer_experimental/test/services/formatter_test.dart (right): https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/tes... pkg/analyzer_experimental/test/services/formatter_test.dart:51: On 2013/06/19 22:12:05, scheglov wrote: > There was no empty line in the same code in test before. > Do we need it? Maybe not. Consolidating tests. Thanks! https://codereview.chromium.org/17470004/diff/1/pkg/analyzer_experimental/tes... pkg/analyzer_experimental/test/services/formatter_test.dart:111: expect(doFormat(recorder).length, equals((src+ NEW_LINE).length)); On 2013/06/19 22:12:05, scheglov wrote: > Space before '+'. Done.
Message was sent while issue was closed.
Committed patchset #1 manually as r24255 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||
