This repository was archived by the owner on Nov 20, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 166
[wip] Introduce DartDoc "Summary Line" lint #3820
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
9c5393a
Adds dartdoc_summary_line lint
ditman e6dc84f
Wires the new lint into the CLI.
ditman 07b1ed5
Remove slash-star-star code path. Address PR comments.
ditman 799cc59
Mark the beginning of the comment rather than the whole line when the…
ditman ec3a0c0
Locate separate sentences in different extracted tokens.
ditman f14d462
Simplify things a lot. Do not trip on periods in the middle of the line.
ditman File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| // Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file | ||
| // for details. All rights reserved. Use of this source code is governed by a | ||
| // BSD-style license that can be found in the LICENSE file. | ||
|
|
||
| import 'package:analyzer/dart/ast/ast.dart'; | ||
| import 'package:analyzer/dart/ast/token.dart'; | ||
| import 'package:analyzer/dart/ast/visitor.dart'; | ||
|
|
||
| import '../analyzer.dart'; | ||
|
|
||
| const _desc = | ||
| r'Start your DartDoc comment with a single, brief sentence that ends with a period.'; | ||
|
|
||
| const _details = r''' | ||
| From [Effective Dart](https://dart.dev/guides/language/effective-dart/documentation#do-start-doc-comments-with-a-single-sentence-summary): | ||
|
|
||
| **DO** start doc comments with a single-sentence summary. | ||
|
|
||
| Start your doc comment with a brief, user-centric description ending with a | ||
| period. A sentence fragment is often sufficient. Provide just enough context | ||
| for the reader to orient themselves and decide if they should keep reading or | ||
| look elsewhere for the solution to their problem. | ||
|
|
||
| For example: | ||
|
|
||
| **GOOD:** | ||
| ```dart | ||
| /// Deletes the file at [path] from the file system. | ||
| void delete(String path) { | ||
| ... | ||
| } | ||
| ``` | ||
|
|
||
| **BAD:** | ||
| ```dart | ||
| /// Depending on the state of the file system and the user's permissions, | ||
| /// certain operations may or may not be possible. If there is no file at | ||
| /// [path] or it can't be accessed, this function throws either [IOError] | ||
| /// or [PermissionError], respectively. Otherwise, this deletes the file. | ||
| void delete(String path) { | ||
| ... | ||
| } | ||
| ``` | ||
|
|
||
| From [Effective Dart](https://dart.dev/guides/language/effective-dart/documentation#do-separate-the-first-sentence-of-a-doc-comment-into-its-own-paragraph): | ||
|
|
||
| **DO** separate the first sentence of a doc comment into its own paragraph. | ||
|
|
||
| Add a blank line after the first sentence to split it out into its own | ||
| paragraph. If more than a single sentence of explanation is useful, put the | ||
| rest in later paragraphs. | ||
|
|
||
| This helps you write a tight first sentence that summarizes the documentation. | ||
| Also, tools like dart doc use the first paragraph as a short summary in places | ||
| like lists of classes and members. | ||
|
|
||
| For example: | ||
|
|
||
| **GOOD:** | ||
| ```dart | ||
| /// Deletes the file at [path]. | ||
| /// | ||
| /// Throws an [IOError] if the file could not be found. Throws a | ||
| /// [PermissionError] if the file is present but could not be deleted. | ||
| void delete(String path) { | ||
| ... | ||
| } | ||
| ``` | ||
|
|
||
| **BAD:** | ||
| ```dart | ||
| /// Deletes the file at [path]. Throws an [IOError] if the file could not | ||
| /// be found. Throws a [PermissionError] if the file is present but could | ||
| /// not be deleted. | ||
| void delete(String path) { | ||
| ... | ||
| } | ||
| ``` | ||
| '''; | ||
|
|
||
| /// Enforces that DartDoc blocks start with a single line that ends in a period. | ||
| /// | ||
| /// (Like the one above!) | ||
| class DartdocSummaryLine extends LintRule { | ||
| DartdocSummaryLine() | ||
| : super( | ||
| name: 'dartdoc_summary_line', | ||
| description: _desc, | ||
| details: _details, | ||
| group: Group.style); | ||
|
|
||
| @override | ||
| void registerNodeProcessors( | ||
| NodeLintRegistry registry, LinterContext context) { | ||
| var visitor = _Visitor(this); | ||
| registry.addComment(this, visitor); | ||
| } | ||
| } | ||
|
|
||
| class _Visitor extends SimpleAstVisitor<void> { | ||
| final LintRule rule; | ||
|
|
||
| _Visitor(this.rule); | ||
|
|
||
| /// The summary line should be 'brief' according to the Dart recommendations, | ||
| /// however 'brief' is not defined perfectly. Instead of enforcing a fixed | ||
| /// number of characters, we just look at how many lines are part of the | ||
| /// summary, and if we deem them to be too many (2 should be plenty), we fail | ||
| /// the lint | ||
| /// | ||
| /// The above summary fails with all the checks that this lint adds, it has | ||
| /// more than one sentence, is too long, and doesn't add a period at the end. | ||
| /// | ||
| /// It has it all! | ||
| @override | ||
| void visitComment(Comment node) { | ||
| if (!node.isDocumentation || !node.tokens.first.lexeme.startsWith('///')) { | ||
| // Ignore /** comments. | ||
| return; | ||
| } | ||
|
|
||
| Iterable<Token> summary = _getSummary(node); | ||
| if (summary.length > 1) { | ||
| int length = summary.last.charEnd - summary.first.charOffset; | ||
| rule.reportLintForOffset(summary.first.charOffset, length); | ||
| return; | ||
| } | ||
|
|
||
| Token token = summary.last; // and only | ||
| if (!_endsWithPeriod(token)) { | ||
| rule.reportLintForToken(token); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Some utility functions | ||
|
|
||
| /// Removes the first triple slash of a `line` and returns its trimmed value. | ||
| String _trimSlashes(String line) => line.replaceFirst('///', '').trim(); | ||
|
|
||
| /// Determines if a token `tk` represents an empty comment line. | ||
| bool _isNotBlankLine(Token tk) => _trimSlashes(tk.lexeme).isNotEmpty; | ||
|
|
||
| /// Retrieves the summary line of a [Comment] `node`. | ||
| /// | ||
| /// This will take tokens (each comment line) until an empty line is found. | ||
| Iterable<Token> _getSummary(Comment node) => | ||
| node.tokens.takeWhile(_isNotBlankLine); | ||
|
|
||
| /// Checks that the last of the `lines` ends in a period. | ||
| bool _endsWithPeriod(Token token) => token.lexeme.endsWith('.'); | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 think we need some data on this point. If plenty of "fine" doc comments exist which are written across more than 2 lines, we'll have to adjust, so that the rule can be used.
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.
Since part of this is geared towards the generated Dartdoc... Do we have examples where the generated dartdoc looks good/bad because of the summary, and try to infer what's a good limit here? IIRC Dartdoc trimmed this at some point? (or I remember a [...] which pointed to the full doc page or something?)
If unlimited, people can write walls of text as the "first line" of the dartdoc, and that kind of defeats the purpose of the lint I think :/
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.
Drive by: Given that it's important for the lint and for dartdoc to agree, have we considered moving the logic that dartdoc uses to find the summary into the
markdownpackage (or maybe even to theanalyzerpackage as a getter onComment) so that it can be shared? Having the logic in two places opens the door for them becoming inconsistent in the future.Uh oh!
There was an error while loading. Please reload this page.
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.
dartdoc takes the inner HTML of the first HTML node of the rendered documentation. We could move this logic to be shared, but it requires dartdoc's custom resolution logic, so we'd have to move that too.
https://github.com/dart-lang/dartdoc/blob/master/lib/src/render/documentation_renderer.dart#L61
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.
So how does it decide what to include in the first HTML node?
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.
It parses the documentation comment and renders it into HTML. So the user determines what is in the first HTML node.
In this markdown:
the first HTML node is
<h1>header</h1>, whose inner HTML isheader. If the first HTML node is a<p>node with a gigabyte of HTML inside, then that is the first node, and the gigabyte of HTML is displayed.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.
Right. I should probably drop out of the conversation with apologies for distracting everyone. It just seemed to me that the logic dartdoc / markdown uses to determine what's in the first HTML node could be used without needing to first build the HTML and then pull it back out. But if it isn't worth the effort then it isn't.