Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16562012:
Dart formatter babysteps. (Closed)
|
Created:
7 years, 6 months ago by pquitslund Modified:
7 years, 6 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionDart formatter babysteps.
Largely scaffolding and basic datastructures. Very much subject to change.
Dart style pointers greatly appreciated! :)
R=brianwilkerson@google.com, messick@google.com, rnystrom@google.com
Committed: https://code.google.com/p/dart/source/detail?r=23769
Patch Set 1 #
Total comments: 54
Messages
Total messages: 8 (0 generated)
Not much here as of yet but enough to put a stake on the ground and solicit some style direction. Thanks in advance! :)
LGTM https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... File pkg/analyzer_experimental/lib/src/services/formatter_impl.dart (right): https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:129: return scanner.tokenize(); I noticed that you're testing for new-lines in the EditRecorder. You might want to capture the line information from the scanner because this information has probably been computed for you. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:202: abstract class EditStore { Edits and edit stores sound like a re-usable piece that should be in their own library.
A few nits, but the style is otherwise quite nice. Looks like you're about 85% of the way along the Java->Dart programmer path. :) LGTM for style after these suggestions. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... File pkg/analyzer_experimental/lib/src/services/formatter.dart (right): https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter.dart:7: import 'package:analyzer_experimental/src/services/formatter_impl.dart'; Why the separation? Why not just move stuff directly into here? https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... File pkg/analyzer_experimental/lib/src/services/formatter_impl.dart (right): https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:8: import 'dart:io'; Nit, but Nathan and I put organize our imports like: 1. "dart:" imports. 2. Blank line. 3. "package:" imports. 4. Blank line. 5. Relative imports. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:12: import 'package:analyzer_experimental/src/generated/source.dart'; These are fine, but you can also use relative imports for these if you prefer. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:16: const String NEW_LINE = '\n' ; //Platform.pathSeparator; Ditch the type annotation. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:23: const FormatterOptions({this.initialIndentationLevel:0, Space after ":". https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:78: int indentationLevel:0}); Indent a line continuation +4, not +2. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:82: class CodeFormatterImpl implements CodeFormatter, AnalysisErrorListener { Unify this with CodeFormatter. Simpler = better. :) https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:103: ASTNode parse(CodeKind kind, Token start) { The style guide would name this "AstNode". https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:123: void onError(AnalysisError error){ Space before "{". https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:186: for (var i = 0; i < NEW_LINE.length; i++) { Do you need to increment index here too? https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:196: final String SPACE = ' '; "final String" -> "const". https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:216: Edit getLastEdit(); You'll already have edits.last, so I'd ditch this. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:228: class BasicEditStore implements EditStore { You can unify this with EditStore. If users don't want to inherit its concrete implementation, they can always just implement instead of extending it. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:230: final List<Edit> edits = new List<Edit>(); final edits = <Edit>[]; https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:256: } Unindent.
LGTM Great to see this! https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... File pkg/analyzer_experimental/lib/src/services/formatter_impl.dart (right): https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:15: /// OS line separator. (TODO: may not be necessary) TODO(ldap) everywhere? It probably seems redundant but I think many team members expect to see the ldap. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:70: abstract class CodeFormatter { I'm not clear what your plan is but I wasn't expecting to see an abstract formatter class. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:103: ASTNode parse(CodeKind kind, Token start) { We should talk to Konstantin to see if we can get ASTNode renamed. I wonder how many people are depending on it already... https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:111: return parser.parseStatement(start); At a guess, I'd expect refactoring to want a few more of these: directive list, statement list (or block), expression. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:136: class Void extends Object { I hope you'll be able to use Keyword.VOID instead of this. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:177: indentationLevel += options.indentPerLevel; These names are confusing. What's a "level"? indentationLevel indentPerLevel https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:202: abstract class EditStore { On 2013/06/06 22:21:58, Brian Wilkerson wrote: > Edits and edit stores sound like a re-usable piece that should be in their own > library. I think so, too. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:241: //TODO(pquitslund): verify that this should be "last" vs. "next" I wonder if this comment is in the right place? https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/tes... File pkg/analyzer_experimental/test/services/formatter_test.dart (right): https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/tes... pkg/analyzer_experimental/test/services/formatter_test.dart:13: group('edit recorder', () { I'd add a few tests that are expected to fail.
Thanks a million all! I think Bob's assessment of me being 85% there is overly generous but I should get there eventually. In the meantime, thanks for the sage advice! https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... File pkg/analyzer_experimental/lib/src/services/formatter.dart (right): https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter.dart:7: import 'package:analyzer_experimental/src/services/formatter_impl.dart'; On 2013/06/06 22:23:06, Bob Nystrom wrote: > Why the separation? Why not just move stuff directly into here? My thinking was so that I can choose what abstractions to export as API without having to resort to underscoring all of my type names. But maybe there's a better way? https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... File pkg/analyzer_experimental/lib/src/services/formatter_impl.dart (right): https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:8: import 'dart:io'; On 2013/06/06 22:23:06, Bob Nystrom wrote: > Nit, but Nathan and I put organize our imports like: > > 1. "dart:" imports. > 2. Blank line. > 3. "package:" imports. > 4. Blank line. > 5. Relative imports. Done. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:12: import 'package:analyzer_experimental/src/generated/source.dart'; On 2013/06/06 22:23:06, Bob Nystrom wrote: > These are fine, but you can also use relative imports for these if you prefer. I thought this would lead to woes if I ever wanted to debug in dartium? I vaguely recall that this was the crux of some issues were seeing with stepping. Again: I'm probably mis-remembering. Or it's been since fixed! https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:15: /// OS line separator. (TODO: may not be necessary) On 2013/06/07 13:51:36, messick wrote: > TODO(ldap) everywhere? It probably seems redundant but I think many team members > expect to see the ldap. Done. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:16: const String NEW_LINE = '\n' ; //Platform.pathSeparator; On 2013/06/06 22:23:06, Bob Nystrom wrote: > Ditch the type annotation. Oh my. This is taking some getting used to. :) Done! https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:23: const FormatterOptions({this.initialIndentationLevel:0, On 2013/06/06 22:23:06, Bob Nystrom wrote: > Space after ":". Done. If only I had a formatter to do this for me! https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:70: abstract class CodeFormatter { On 2013/06/07 13:51:36, messick wrote: > I'm not clear what your plan is but I wasn't expecting to see an abstract > formatter class. See response to Bob below. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:78: int indentationLevel:0}); On 2013/06/06 22:23:06, Bob Nystrom wrote: > Indent a line continuation +4, not +2. Done. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:82: class CodeFormatterImpl implements CodeFormatter, AnalysisErrorListener { On 2013/06/06 22:23:06, Bob Nystrom wrote: > Unify this with CodeFormatter. Simpler = better. :) OK. I *might*. My thinking here is that I really want the API surface area of the CodeFormatter consumed by the public to be concise. As such I don't want them seeing implementation details (e.g., implements AnalysisErrorListener and so on). This might be unnecessary but at least for now I want to lean this way. But feel free and try and unconvince me! https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:103: ASTNode parse(CodeKind kind, Token start) { On 2013/06/06 22:23:06, Bob Nystrom wrote: > The style guide would name this "AstNode". Aha. Something for Konstantin (or Brian). Thanks! https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:103: ASTNode parse(CodeKind kind, Token start) { On 2013/06/07 13:51:36, messick wrote: > We should talk to Konstantin to see if we can get ASTNode renamed. I wonder how > many people are depending on it already... Good point! https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:111: return parser.parseStatement(start); On 2013/06/07 13:51:36, messick wrote: > At a guess, I'd expect refactoring to want a few more of these: directive list, > statement list (or block), expression. Absolutely. Need to crawl before I can walk though! :) https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:123: void onError(AnalysisError error){ On 2013/06/06 22:23:06, Bob Nystrom wrote: > Space before "{". Eagle eye. Fixed! https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:129: return scanner.tokenize(); On 2013/06/06 22:21:58, Brian Wilkerson wrote: > I noticed that you're testing for new-lines in the EditRecorder. You might want > to capture the line information from the scanner because this information has > probably been computed for you. Cool. Great tip. I'll look into that. Thanks! https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:136: class Void extends Object { On 2013/06/07 13:51:36, messick wrote: > I hope you'll be able to use Keyword.VOID instead of this. The issue is that I really like to use VOID as a type parameter when implementing visitors. Maybe I'm missing something but I'm not sure how how Keyword.VOID would work for that. Or? https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:177: indentationLevel += options.indentPerLevel; On 2013/06/07 13:51:36, messick wrote: > These names are confusing. What's a "level"? > indentationLevel > indentPerLevel Working on it. Thanks! https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:186: for (var i = 0; i < NEW_LINE.length; i++) { On 2013/06/06 22:23:06, Bob Nystrom wrote: > Do you need to increment index here too? Time for some more tests! :) https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:196: final String SPACE = ' '; On 2013/06/06 22:23:06, Bob Nystrom wrote: > "final String" -> "const". Done. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:216: Edit getLastEdit(); On 2013/06/06 22:23:06, Bob Nystrom wrote: > You'll already have edits.last, so I'd ditch this. Right but for some reason I convinced myself that I'd like to return null if the edit list is empty (rather than throwing an exception). I *think* this makes it easier on callers. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:228: class BasicEditStore implements EditStore { On 2013/06/06 22:23:06, Bob Nystrom wrote: > You can unify this with EditStore. If users don't want to inherit its concrete > implementation, they can always just implement instead of extending it. Ah yes. Good point! Done. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:230: final List<Edit> edits = new List<Edit>(); On 2013/06/06 22:23:06, Bob Nystrom wrote: > final edits = <Edit>[]; Done. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:241: //TODO(pquitslund): verify that this should be "last" vs. "next" On 2013/06/07 13:51:36, messick wrote: > I wonder if this comment is in the right place? Done. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:256: } On 2013/06/06 22:23:06, Bob Nystrom wrote: > Unindent. Nothing escapes you! https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/tes... File pkg/analyzer_experimental/test/services/formatter_test.dart (right): https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/tes... pkg/analyzer_experimental/test/services/formatter_test.dart:13: group('edit recorder', () { On 2013/06/07 13:51:36, messick wrote: > I'd add a few tests that are expected to fail. Will do. Thanks!
Message was sent while issue was closed.
Committed patchset #1 manually as r23769 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... File pkg/analyzer_experimental/lib/src/services/formatter_impl.dart (right): https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:82: class CodeFormatterImpl implements CodeFormatter, AnalysisErrorListener { > As such I don't want them seeing implementation details. Another alternative would be to not implement AnalysisErrorListener, since it's not really how you want a code formatter to be used, and to have a separate object that gathers errors for you. There is a RecordingErrorListener in the resolver and a GatheringErrorListener in the test framework (you couldn't use it directly, but you could take it as a starting point for implementing your own).
Message was sent while issue was closed.
https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... File pkg/analyzer_experimental/lib/src/services/formatter.dart (right): https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter.dart:7: import 'package:analyzer_experimental/src/services/formatter_impl.dart'; On 2013/06/07 19:45:01, pquitslund wrote: > On 2013/06/06 22:23:06, Bob Nystrom wrote: > > Why the separation? Why not just move stuff directly into here? > > My thinking was so that I can choose what abstractions to export as API without > having to resort to underscoring all of my type names. But maybe there's a > better way? Yeah, I guess that works. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... File pkg/analyzer_experimental/lib/src/services/formatter_impl.dart (right): https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:12: import 'package:analyzer_experimental/src/generated/source.dart'; On 2013/06/07 19:45:01, pquitslund wrote: > On 2013/06/06 22:23:06, Bob Nystrom wrote: > > These are fine, but you can also use relative imports for these if you prefer. > > I thought this would lead to woes if I ever wanted to debug in dartium? I > vaguely recall that this was the crux of some issues were seeing with stepping. > Again: I'm probably mis-remembering. Or it's been since fixed! It's very confusing, unfortunately. The basic rule is you can use relative imports from stuff under lib/ to stuff under that same lib/ directory. Anything that reaches out of or into a lib/ directory should use package:. https://codereview.chromium.org/16562012/diff/1/pkg/analyzer_experimental/lib... pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:216: Edit getLastEdit(); On 2013/06/07 19:45:01, pquitslund wrote: > On 2013/06/06 22:23:06, Bob Nystrom wrote: > > You'll already have edits.last, so I'd ditch this. > > Right but for some reason I convinced myself that I'd like to return null if the > edit list is empty (rather than throwing an exception). I *think* this makes it > easier on callers. They could just do edits.isEmpty to check for that case. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
