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

Expand on mixin class #4834

Merged
merged 5 commits into from
May 11, 2023
Merged

Expand on mixin class #4834

merged 5 commits into from
May 11, 2023

Conversation

MaryaBelanger
Copy link
Contributor

@MaryaBelanger MaryaBelanger commented May 9, 2023

Fixes #4826

I had trouble making the connection between what the on clause is for and how using abstract provides the same benefit. Here's what I think the reason is (added to mixins.md), but I could be off:

By declaring Musician abstract, you force any type that uses
it to define the abstract method its behavior depends on.

This is similar to how the on directive ensures a mixin has access to any
interfaces it depends on by specifying the superclass of that interface.

*Also not sure if I'mm using "interfaces" correctly there.

I assumed that this would be a common use case, so I put this explanation in its own subsection on the mixin page, "abstract mixin class".

@MaryaBelanger MaryaBelanger self-assigned this May 9, 2023
@parlough parlough added the review.tech Awaiting Technical Review label May 9, 2023
@github-actions
Copy link

github-actions bot commented May 9, 2023

Visit the preview URL for this PR (updated for commit 01c9f4b):

https://dart-dev--pr4834-class-modifiers-2-4nrn8v3y.web.app

(expires Thu, 18 May 2023 21:06:16 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: d851bc446d3c4d7394c5406c6f07255afc7075f3

@MaryaBelanger
Copy link
Contributor Author

This is pointing at v3.0 but if it can't get reviewed before merging that into main tomorrow morning, it'll be fine to merge this directly into main later on.

src/language/mixins.md Show resolved Hide resolved
src/language/mixins.md Outdated Show resolved Hide resolved
src/language/mixins.md Outdated Show resolved Hide resolved
src/language/mixins.md Outdated Show resolved Hide resolved

### `abstract mixin class`

You can achieve similar behavior to the `on` directive for a mixin class.
Copy link
Member

Choose a reason for hiding this comment

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

on clauses are really confusing. Fortunately, almost no one actually needs to use them.

People often think they are used to specify the methods than a mixin can call on itself and then get implemented by the thing that applies the mixin. That's what your example is about here.

But you don't need an on clause for that at all. As you do here, you can just make the type abstract and define abstract methods. Another way to accomplish something similar is to put an implements clause on the mixin or mixin class and then not actually implement the interface.

The only time you need an on clause is when the mixin wants to do super calls. It specifies the interface that those are resolved against. So in:

mixin M {}

class B {}
class C extends B with M {}

If there's some method in C that you want to be able to call from M, you can just declare it abstract in M and call it, like in your example:

mixin M {
  void c(); // Declare abstract.
  void test() {
    c();
  }
}

class B {}
class C extends B with M {
  void c() { ... }
}

You need an on clause if M wants to have a super call that needs to be resolved against B in the example here:

abstract class BStuff {
  void b();
}

mixin M on BInterface {
  void test() {
    super.b();
  }
}

class B {
  void b() { ... }
}

class C extends B with M {}

Honestly, on clauses and super calls in mixins are just not a great feature. Almost any time you think you need one, there's probably a better design you should use instead. But Flutter used super calls in a couple of key mixins early on (because the VM allowed it even though the language didn't specify it), so we were sort of forced to turn it into a real feature. But I definitely wouldn't emphasize it in the tour since it tends to confuse users and it's rarely what they want.

Copy link
Member

Choose a reason for hiding this comment

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

Summarizing: I think your example is OK here, but I wouldn't mention it in the context of on clauses. If that makes the entire example not worth keeping, I think it's fine to eliminate this section.

Copy link
Contributor

Choose a reason for hiding this comment

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

The text before this section contains these sentences:

Sometimes you might want to restrict the types that can use a mixin.
For example, the mixin might depend on being able to invoke a method
that the mixin doesn't define.

That is confusing given what you just said. There might not be time for this but it might make sense to reorder this page i.e. have an example of calling the mixing classes method from the mixin near the top (where the on example currently is), then have a section about class mixin and then finally a section explaining on with the "you will almost certainly never need" caveat and an explanation that it is only useful for super calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm.. ok, I think what I'll do for now is keep the new section referencing similarity to using on, but more of an aside than the purpose statement it currently is. Like Brian said, this is a direct response to the earlier text describing on.

Later* I'll rewrite the section to take out any emphasis on on and instead describe the same example for both mixins and mixin classes simply as "the use case where you want to require that whatever uses the mixin specifies some methods".

*Only delaying it because, again like Brian said, I think it'd be best to rearrange the page, since ...

  • this isn't just a use case for mixin classes,
  • and, like Bob said, the existing description of on for regular mixins seems misrepresented and could probably use a rewrite

And doing that will take a bit of attention that I won't have before publishing the new pages. I'd like to include this info on the first publish. @munificent if you think the content just absolutely shouldn't be published, please let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

My two cents: It's worth mentioning in passing for coverage. It should be reviewed in the next week or so to see if it's truly relevant. I'm a fan of being opinionated in docs, even if the opinion is to skip something entirely.

parlough added a commit that referenced this pull request May 10, 2023
Mostly pulled from #4834 since
not all of it has been decided on.

I also added a few other fixes related to switches while I was at it.

---------

Co-authored-by: Marya <111139605+MaryaBelanger@users.noreply.github.com>
@parlough parlough changed the title Expand on mixin class and sealed Expand on mixin class May 10, 2023
@parlough
Copy link
Member

I pulled out the sealed changes in #4847 so I could land those and other fixes and so discussion can continue on mixins here.

@parlough parlough changed the base branch from v3.0 to main May 10, 2023 13:31
@parlough
Copy link
Member

I've changed the base branch to main since the v3.0 branch landed in main.

@MaryaBelanger
Copy link
Contributor Author

@atsansone could you approve this to get the information out for now, so I can come back and apply Bob's concerns later on?

Copy link
Contributor

@atsansone atsansone left a comment

Choose a reason for hiding this comment

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

One issue, otherwise LGTM. No need for a second pass.

src/language/mixins.md Outdated Show resolved Hide resolved
src/language/mixins.md Outdated Show resolved Hide resolved
src/language/mixins.md Show resolved Hide resolved

### `abstract mixin class`

You can achieve similar behavior to the `on` directive for a mixin class.
Copy link
Contributor

Choose a reason for hiding this comment

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

My two cents: It's worth mentioning in passing for coverage. It should be reviewed in the next week or so to see if it's truly relevant. I'm a fan of being opinionated in docs, even if the opinion is to skip something entirely.

@atsansone atsansone added st.RFM.% Ready to Merge but has suggestions and removed review.tech Awaiting Technical Review labels May 10, 2023
MaryaBelanger and others added 2 commits May 11, 2023 13:49
Co-authored-by: Anthony Sansone <atsansone@users.noreply.github.com>
@MaryaBelanger MaryaBelanger merged commit d160f38 into main May 11, 2023
9 checks passed
@parlough parlough deleted the class-modifiers-2 branch May 17, 2023 18:50
rmacnak-google pushed a commit to rmacnak-google/site-www that referenced this pull request Sep 5, 2023
Fixes dart-lang#4826 

Co-authored-by: Anthony Sansone <atsansone@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
st.RFM.% Ready to Merge but has suggestions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feedback on Dart 3 mixins and class modifier documentation
5 participants