Skip to content

Remove unnecessary Builder classes #3664

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

Merged
merged 7 commits into from
Feb 22, 2024

Conversation

srawlins
Copy link
Member

Big ball of yarn that grew and grew.

The main idea is to remove a bunch of classes, whose necessity I never understood: ElementTypeBuilder (and Impl), ModelElementBuilder (and Impl), ModelObjectBuilder, ModelBuilder, and ModelBuilderInterface. These might have had more of a purpose when we could output to HTML or Markdown, but their complexity is now unwarranted.

The purpose they served is this: In order to fetch a ModelElement or ElementType from from the package graph, you always needed an instance of... the package graph. Putting a *Builder* in a class you're working in gave you a way to instantiate (or fetch from the package graph) a ModelElement or ElementType.

But, lo and behold, we can declare that a Nameable must have a PackageGraph, and then write little utility functions to fetch these things. Much simpler. Here are the other changes needed to facilitate this:

  • I renamed the various factory constructors. I generally changed the word 'from' to 'for', because often the value that the factory constructor returns is a cached instance. It feels weird to me to say ModelElement.from some Element, when you know that you already have the instance, and you're just fetching it, using the Element as a key. In that case, I think 'for' is more appropriate.

    • ModelElement._from --> ModelElement.for_
    • ModelElement._fromElement --> ModelElement.forElement
    • ModelElement._fromPropertyInducingElement --> ModelElement.forPropertyInducingElement
    • ElementType._from --> ElementType.for_
    • These did have to be made public, which is unfortunate; I think there are some gymnastics we can do to re-privatize them, but it would more-or-less mean moving Nameable, or a Nameable supertype... I think not worth it.
  • Make Nameable a proper mixin. 👍

  • Move _resolveMultiplyInheritedElement to be an extension method, to the bottom of the library.

I believe this may also open up nicer refactorings where some late final ElementType fields do not need to be late. It might not be a hole in one, but once we're only relying on packageGraph, we may be able to assign some more instance fields at construction-time.


  • 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 21, 2024 14:16
Copy link
Member

@kallentu kallentu left a comment

Choose a reason for hiding this comment

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

Thank you for doing this. I saw the original ModelBuilder mixins and impls and I wanted to fix it up, so I'm glad you did.

@srawlins srawlins merged commit ec26cee into dart-lang:main Feb 22, 2024
@srawlins srawlins deleted the tidy-fromPropertyInducingElement branch February 22, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants