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

Centralize unparsing of comment references #2630

Merged
merged 6 commits into from
Apr 30, 2021

Conversation

jcollins-g
Copy link
Contributor

@jcollins-g jcollins-g commented Apr 28, 2021

Before implementing new parsing of comment references, we should clean up how dartdoc gets comment references in the first place. Unparsing them is now centralized which should make it more clear how the analyzer AST could help dartdoc by providing the raw string.

Breaking change: commentRefs, a member of ModelElements, is a map now instead of a list.

@google-cla google-cla bot added the cla: yes Google CLA check succeeded. label Apr 28, 2021
Copy link
Member

@srawlins srawlins left a comment

Choose a reason for hiding this comment

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

Sorry, I know just a draft, but I couldn't help but look!

@@ -0,0 +1,29 @@
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
Copy link
Member

Choose a reason for hiding this comment

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

2021?

@@ -126,10 +126,6 @@ final RegExp nonHTML =
/// parentheses.
final _trailingIgnorePattern = RegExp(r'(<.*>|\(.*\)|\?|!)$');

@Deprecated('Public variable intended to be private; will be removed as early '
Copy link
Member

Choose a reason for hiding this comment

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

Woohoo!

@@ -369,26 +357,25 @@ MatchingLinkResult _getMatchingLinkElementLegacy(String codeRef,
}

/// Given a set of commentRefs, return the one whose name matches the codeRef.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the doc comment doesn't apply any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrote a more complete doc.

/// [CommentReference].
static String _referenceText(
CommentReference ref, ResourceProvider resourceProvider) {
var contents = getFileContentsFor(
Copy link
Member

Choose a reason for hiding this comment

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

Is this super expensive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fortunately not. It is cached so we only pay a read penalty once per file.

@coveralls
Copy link

coveralls commented Apr 28, 2021

Coverage Status

Coverage decreased (-0.03%) to 57.571% when pulling 2786435 on jcollins-g:commentref-unparse into bf7eb56 on dart-lang:master.

@@ -0,0 +1,29 @@
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed

@@ -369,26 +357,25 @@ MatchingLinkResult _getMatchingLinkElementLegacy(String codeRef,
}

/// Given a set of commentRefs, return the one whose name matches the codeRef.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrote a more complete doc.

@jcollins-g jcollins-g marked this pull request as ready for review April 30, 2021 18:50
@jcollins-g
Copy link
Contributor Author

I have redone some benchmarks before marking this as ready for review, to make sure the changes to comment reference handling didn't harm performance. Fortunately, the delta is negligible.

@jcollins-g jcollins-g merged commit bf115eb into dart-lang:master Apr 30, 2021
@jcollins-g jcollins-g deleted the commentref-unparse branch April 30, 2021 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants