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

Include extension types in 'implementers' list #3658

Merged
merged 3 commits into from
Feb 16, 2024
Merged

Include extension types in 'implementers' list #3658

merged 3 commits into from
Feb 16, 2024

Conversation

srawlins
Copy link
Member

@srawlins srawlins commented Feb 15, 2024 •
edited
Loading

Fixes #3656

I'll get to the punchline quickly: The gist of this fix is a single line in package_graph.dart: _addToImplementers(library.extensionTypes);. The rest is mostly test infra, tech debt cleanup, and yak-shaving:

  • There were no template tests that validate what goes into the implementers section, so I added class_test.dart to introduce such tests.
  • addImplementer(), which must now also consider extension types, did so many as InheritingContainer calls that I decided to add some helpers to do all of this casting as part of this or that class's responsibility, instead of the responsibility of addImplementor(). So mixedInTypes now has sibling mixedInElements, and publicInterfaces has publicInterfaceElements, etc. I found most of the callers concerned themselves with the elements, so I changed most callers to use mixedInElements. Same with implementors etc.
  • I made mixedInTypes @visibleForTesting since it is almost entirely replaced by mixedInElements. But it was still used in templates.runtime_renderers.dart until I corrected the codegen to account for a PropertyAccessorElement's variable.
  • 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 srawlins requested a review from kallentu February 15, 2024 21:33
kallentu
kallentu approved these changes Feb 16, 2024
View reviewed changes
lib/src/model/inheriting_container.dart
.findCanonicalFor(
packageGraph.implementors[implementor] ?? const [])
.forEach(addToResult);
var implementors = packageGraph.implementors[implementor];
Copy link
Member

Choose a reason for hiding this comment

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

Could we name this something slightly different from line 545 (or the opposite)? My eyes are glazing over see them because they're so similar haha.

Copy link
Member

Choose a reason for hiding this comment

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

And also keep implementor vs implementer spelling consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've renamed the lower one immediateImplementors.

It looks like everything in this file is implementor; the display name in the HTML is "implementer", which I think is more correct. But to change everything in this file would make the diff even bigger. I'd like to address that in a follow-up.

test/templates/class_test.dart Outdated
return resourceProvider.readLines([packagePath, 'doc', ...filePath]);
}

void test_pageListsClassesThatExtend() async {
Copy link
Member

Choose a reason for hiding this comment

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

For all the tests, maybe group them like test_class_extends test_class_implements test_mixin_constraintsSuperclass?

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

test/templates/class_test.dart Outdated
]);
}

@FailingTest(reason: 'Not implemented yet; should be?')
Copy link
Member

Choose a reason for hiding this comment

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

This isn't? That's surprising haha.

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 I wrote the test because I assumed it worked. :/

@srawlins srawlins merged commit e8b8faa into dart-lang:main Feb 16, 2024
9 checks passed
@srawlins srawlins deleted the extension-types-implement branch February 16, 2024 20:07
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Feb 21, 2024
@devoncarew
[deps] rev async, browser_launcher, dartdoc, http, markdown, protobuf…
7c223d8 
…, sse, stream_channel, tools, vector_math, web, webdev, yaml_edit

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

async (https://github.com/dart-lang/async/compare/24266ca..6cdbc41):
  6cdbc41  2024-02-15  Kevin Moore  Update to latest lints, require Dart 3.2 (dart-archive/async#267)

browser_launcher (https://github.com/dart-lang/browser_launcher/compare/74a0efe..7956230):
  7956230  2024-02-16  sigmundch  Add extra flags to disable throttling behavior. (dart-archive/browser_launcher#55)

dartdoc (https://github.com/dart-lang/dartdoc/compare/7e171fc..7a9df65):
  7a9df65f  2024-02-20  Parker Lougheed  Add fallback text for sidebar failing to load (dart-lang/dartdoc#3643)
  9bcabb50  2024-02-20  Parker Lougheed  Fix missing left sidebar on extension type pages (dart-lang/dartdoc#3662)
  e8b8faa2  2024-02-16  Sam Rawlins  Include extension types in 'implementers' list (dart-lang/dartdoc#3658)

http (https://github.com/dart-lang/http/compare/6d9f9ef..ce0de37):
  ce0de37  2024-02-21  Derek Xu  Populate package:http_profile (dart-lang/http#1046)
  75e01f4  2024-02-20  Brian Quinlan  Create a simple WebSocket interface (dart-lang/http#1128)

markdown (https://github.com/dart-lang/markdown/compare/c2b8429..d735b0b):
  d735b0b  2024-02-21  Tom Yeh  Fix `#578`: list with checkbox mixed with empty lines (dart-lang/markdown#583)
  6efe141  2024-02-14  Kevin Moore  Migrate example to pkg:web, update minimum required Dart version (dart-lang/markdown#582)

protobuf (https://github.com/dart-lang/protobuf/compare/a293fb9..f085bfd):
  f085bfd  2024-02-20  Ömer Sinan Ağacan  Fix message_set.dart copyright year (google/protobuf.dart#912)

sse (https://github.com/dart-lang/sse/compare/af7d8d0..13ec752):
  13ec752  2024-02-20  Kevin Moore  blast_repo fixes (dart-lang/sse#104)
  2830dc9  2024-02-16  Kevin Moore  Support the latest pkg:web, require Dart 3.3 (dart-lang/sse#103)

stream_channel (https://github.com/dart-lang/stream_channel/compare/851336f..e02a5dd):
  e02a5dd  2024-02-16  Kevin Moore  Require Dart 3.3, update and fix lints (dart-lang/stream_channel#100)
  e62706e  2024-02-16  Kevin Moore  blast_repo fixes (dart-lang/stream_channel#101)

tools (https://github.com/dart-lang/tools/compare/2ef7673..9f4e6a4):
  9f4e6a4  2024-02-16  Elias Yishak  Helper to resolve dart version for clients of analytics (dart-lang/tools#233)
  8323b21  2024-02-13  Elias Yishak  New event added for sending analytics within package on errors (dart-lang/tools#229)

vector_math (https://github.com/google/vector_math.dart/compare/cb976c7..3706feb):
  3706feb  2024-02-18  dependabot[bot]  Bump dart-lang/setup-dart from 1.6.0 to 1.6.2 (google/vector_math.dart#313)

web (https://github.com/dart-lang/web/compare/a54a1f0..975e55c):
  975e55c  2024-02-15  Kevin Moore  Add TrustedTypes (dart-lang/web#173)
  0447807  2024-02-15  Srujan Gaddam  Add info on generation conventions (dart-lang/web#171)

webdev (https://github.com/dart-lang/webdev/compare/629c632..51b5484):
  51b54843  2024-02-14  Elliott Brooks  Implement `setFlag` for 'pause_isolates_on_start' (dart-lang/webdev#2373)

yaml_edit (https://github.com/dart-lang/yaml_edit/compare/2a9a11b..82ab64d):
  82ab64d  2024-02-21  Danny Tuppeny  Fix line endings for inserted maps on Windows (dart-lang/yaml_edit#66)
  6906ac4  2024-02-20  Devon Carew  update the publish workflow (dart-lang/yaml_edit#67)

Change-Id: I246c393586e3d6239925ac3cf3a6a245d86a2bf5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/353581
Reviewed-by: Kevin Moore <[email protected]>
Commit-Queue: Devon Carew <[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.

In "implementers" list, include extension type implementers along with classes and mixins
2 participants
@srawlins @kallentu