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.

Sign up for GitHub

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

Jump to bottom

Tidy some comments #3741

Merged
merged 2 commits into from
Apr 3, 2024
Merged

Tidy some comments #3741

merged 2 commits into from
Apr 3, 2024

Conversation

srawlins
Copy link
Member

@srawlins srawlins commented Apr 3, 2024

  • Move the Locatable.documentationFrom comment from an override to the highest declaration.
  • Make a few comments more idiomatic.
  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@srawlins
Tidy some comments:
6198a91 
* Move the `Locatable.documentationFrom` comment from an override to the
  highest declaration.
* Make a few comments more idiomatic.
@srawlins srawlins requested a review from kallentu April 3, 2024 19:40
kallentu
kallentu approved these changes Apr 3, 2024
View reviewed changes
lib/src/model/locatable.dart Outdated
@@ -6,6 +6,13 @@ import 'package:analyzer/dart/element/element.dart' show Element;

/// Something that can be located for warning purposes.
mixin Locatable {
/// The model element(s) from which we will get documentation.
Copy link
Member

Choose a reason for hiding this comment

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

Why not link [ModelElement] and the other references in backticks like the original comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha, I don't remember why I changed this, but it's not correct anyhow; they're Locatables, including some not-ModelElement things, like Category. Changed.

lib/src/model/inheritable.dart
@@ -125,7 +125,7 @@ mixin Inheritable on ContainerMember {

Inheritable? get overriddenElement;

/// True if this [Inheritable] is overriding a superclass.
/// Whether this [Inheritable] is overriding a member from a superclass.
Copy link
Member

Choose a reason for hiding this comment

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

Can we also mention the relationship between isOverride and isInherited like you mentioned before in GVC? Might be good information for later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

lib/src/model/documentation_comment.dart Outdated
bool get hasDocumentationComment => element.documentationComment != null;

/// Returns true if the raw documentation comment has a 'nodoc' indication.
/// Whether the raw documentation comment has a 'nodoc' indication.
Copy link
Member

Choose a reason for hiding this comment

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

Instead of saying "has a 'nodoc' indication", can we just write out what 'nodoc' means and what it does? Otherwise, someone reading it would have to search to find what 'nodoc' is anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that is weird verbiage. Done!

@srawlins srawlins merged commit 12988e0 into dart-lang:main Apr 3, 2024
9 checks passed
@srawlins srawlins deleted the tidy-comments branch April 3, 2024 22:22
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Apr 9, 2024
@devoncarew
[deps] rev args, bazel_worker, benchmark_harness, dartdoc, ecosystem,…
2fc32d3 
… file, glob, http, http_multi_server, http_parser, json_rpc_2, package_config, protobuf, pub_semver, source_maps, source_span, test, usage, vector_math, web, webdev

Revisions updated by `dart tools/rev_sdk_deps.dart`.

args (https://github.com/dart-lang/args/compare/788d935..b2e2224):
  b2e2224  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-archive/args#266)

bazel_worker (https://github.com/dart-lang/bazel_worker/compare/8619b92..79d2ad1):
  79d2ad1  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-archive/bazel_worker#88)

benchmark_harness (https://github.com/dart-lang/benchmark_harness/compare/c8a0c8b..d23112a):
  d23112a  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-archive/benchmark_harness#101)

dartdoc (https://github.com/dart-lang/dartdoc/compare/bf6080c..28d0dab):
  28d0dabb  2024-04-08  Kallen Tu  Deprecate the `missingCodeBlockLanguage` warning. (dart-lang/dartdoc#3743)
  12988e06  2024-04-03  Sam Rawlins  Tidy some comments (dart-lang/dartdoc#3741)
  2559a774  2024-04-03  Sam Rawlins  Rewrite the 'exported extension method' tests; add basic extension tests. (dart-lang/dartdoc#3738)
  b45174e8  2024-04-03  Sam Rawlins  Correct a small unnecessary chunk in Library.allClasses (dart-lang/dartdoc#3742)
  95305c3e  2024-04-03  Sam Rawlins  Simplify GeneratorFrontend (dart-lang/dartdoc#3739)
  a97508da  2024-04-03  Sam Rawlins  Remove a TODO in Accessor.characterLocation (dart-lang/dartdoc#3740)

ecosystem (https://github.com/dart-lang/ecosystem/compare/5a900ca..de03da1):
  de03da1  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-lang/ecosystem#248)
  001e06f  2024-04-01  dependabot[bot]  Bump actions/download-artifact from 4.1.3 to 4.1.4 (dart-lang/ecosystem#247)
  3dc115b  2024-04-01  dependabot[bot]  Bump subosito/flutter-action from 2.13.0 to 2.15.0 (dart-lang/ecosystem#246)
  7687fb0  2024-04-01  dependabot[bot]  Bump peter-evans/create-or-update-comment (dart-lang/ecosystem#249)
  915840a  2024-04-01  dependabot[bot]  Bump actions/cache from 4.0.0 to 4.0.2 (dart-lang/ecosystem#245)

file (https://github.com/google/file.dart/compare/3aa0649..f858c6f):
  f858c6f  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-archive/file.dart#237)

glob (https://github.com/dart-lang/glob/compare/379d60c..25ee2c2):
  25ee2c2  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-lang/glob#89)

http (https://github.com/dart-lang/http/compare/5214f76..caad9ca):
  caad9ca  2024-04-04  Brian Quinlan  Create a `package:web_socket` `WebSocket` from `dart:io` `WebSocket` (dart-lang/http#1173)

http_multi_server (https://github.com/dart-lang/http_multi_server/compare/ba9d07f..6ce0a13):
  6ce0a13  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-archive/http_multi_server#65)

http_parser (https://github.com/dart-lang/http_parser/compare/84db8b0..8ffcaec):
  8ffcaec  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-archive/http_parser#87)

json_rpc_2 (https://github.com/dart-lang/json_rpc_2/compare/639857b..1a4c473):
  1a4c473  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-archive/json_rpc_2#110)

package_config (https://github.com/dart-lang/package_config/compare/3d90e69..486cc4b):
  486cc4b  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-lang/package_config#150)

protobuf (https://github.com/dart-lang/protobuf/compare/b761358..ccf104d):
  ccf104d  2024-04-01  dependabot[bot]  Bump actions/checkout from 3.6.0 to 4.1.2 (google/protobuf.dart#927)
  15b9c87  2024-04-01  dependabot[bot]  Bump actions/cache from 4.0.1 to 4.0.2 (google/protobuf.dart#926)

pub_semver (https://github.com/dart-lang/pub_semver/compare/3175ba0..7581029):
  7581029  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-lang/pub_semver#99)

source_maps (https://github.com/dart-lang/source_maps/compare/55e92a4..64d07fa):
  64d07fa  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-lang/source_maps#87)

source_span (https://github.com/dart-lang/source_span/compare/21a403a..45e11a3):
  45e11a3  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-lang/source_span#108)

test (https://github.com/dart-lang/test/compare/2b1ed13..14d820f):
  14d820f0  2024-04-08  Nate Bosch  Prepare to publish (dart-lang/test#2207)

usage (https://github.com/dart-lang/usage/compare/67ecd7d..09cab89):
  09cab89  2024-04-02  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-archive/usage#203)

vector_math (https://github.com/google/vector_math.dart/compare/7e705f7..43f2a77):
  43f2a77  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (google/vector_math.dart#318)

web (https://github.com/dart-lang/web/compare/e773de9..3d1b4cb):
  3d1b4cb  2024-04-02  Kashiwa  Fix the links in third_party/mdn/README.md (dart-lang/web#219)

webdev (https://github.com/dart-lang/webdev/compare/4067462..7d0d2d4):
  7d0d2d46  2024-04-05  Elliott Brooks  Can add breakpoints to a project using macros (dart-lang/webdev#2403)
  d715bcd7  2024-04-05  Elliott Brooks  Webdev integration_test should not check for string messages that come from pub (dart-lang/webdev#2405)
  29d6ab8e  2024-04-01  dependabot[bot]  Bump actions/cache from 4.0.1 to 4.0.2 (dart-lang/webdev#2401)
  37491228  2024-04-01  dependabot[bot]  Bump actions/checkout from 4.1.1 to 4.1.2 (dart-lang/webdev#2402)

Change-Id: Id649f7a29dacdba0855c512978feb1d52d4c5313
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/362060
Commit-Queue: Devon Carew <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kallentu kallentu kallentu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@srawlins @kallentu