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

"Specify members" and "access state" for mixins #5694

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

MaryaBelanger
Copy link
Contributor

@MaryaBelanger MaryaBelanger commented Apr 5, 2024

This is an attempt to pick up #4767 and incorporate the this info-packed comment from #4834

It's pretty rough, I wasn't sure if "Specify members a mixin can call on itself" was an appropriate title for the new content, or if "accessing state" is interchangeable topic, or subtopic....?

I'm also totally missing an example (or anything more than a the single sentence about it, pulled from the above-linked comment).

@munificent hopefully this isn't totally off base? Lmk!

Apologies for overriding your work @KyleFin, I was having trouble working on your PR directly with the larger changes I wanted to do. Hopefully you can provide some feedback here!


I think this also fixes #4871

@dart-github-bot
Copy link
Collaborator

dart-github-bot commented Apr 5, 2024

Visit the preview URL for this PR (updated for commit 9282ec6):

https://dart-dev--pr5694-clarify-mixins-ltjhm8cs.web.app

@munificent
Copy link
Member

I'm done for the day and I'll be on vacation all week next week (and then working in AAR the week after that), but I'll take a look at this when I'm back.

@MaryaBelanger MaryaBelanger added review.tech Awaiting Technical Review review.await-update Awaiting Updates after Edits labels Apr 8, 2024
@KyleFin
Copy link
Contributor

KyleFin commented Apr 12, 2024

@MaryaBelanger, this looks great to me! I'm supportive of proceeding here and closing #4767. Thanks for picking this up!

@MaryaBelanger
Copy link
Contributor Author

@munificent Friendly ping :)

Copy link
Member

@munificent munificent left a comment

Choose a reason for hiding this comment

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

One example needs filling in but otherwise LGTM!

dependencies are defined for the mixin.

```dart
?
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to fill this in. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I'm sorry, I think I actually needed help with this example, not a review yet 😬

Would it look something like... this? Not sure what would go in the mixin

class Person {
  final String _name;

  Person(this._name);

  String greet(String who) => 'Hello, $who. I am $_name.';
}

mixin Impersonator implements Person {
  // what goes in here?
}

class Impostor with Impersonator {
  String get _name => '';

  String greet(String who) => 'Hi $who. Do you know who I am?';
}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like:

abstract interface class Tuner {
  void tuneInstrument();
}

mixin Guitarist implements Tuner {
  void playSong() {
    // Should be in tune first!
    tuneInstrument();

    print('Strums guitar majestically.');
  }
}

class PunkRocker implements Tuner with Guitarist {
  void tuneInstrument() {
    print("Don't bother, being out of tune is punk rock.");
  }
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the class using the mixin also needs to implement Tuner, right? I wrote it like class PunkRocker with Guitarist {} and that seems to work fine.

src/content/language/mixins.md Show resolved Hide resolved
src/content/language/mixins.md Show resolved Hide resolved
src/content/language/mixins.md Outdated Show resolved Hide resolved
dependencies are defined for the mixin.

```dart
?
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like:

abstract interface class Tuner {
  void tuneInstrument();
}

mixin Guitarist implements Tuner {
  void playSong() {
    // Should be in tune first!
    tuneInstrument();

    print('Strums guitar majestically.');
  }
}

class PunkRocker implements Tuner with Guitarist {
  void tuneInstrument() {
    print("Don't bother, being out of tune is punk rock.");
  }
}

?

@MaryaBelanger MaryaBelanger added review.copy Awaiting Copy Review and removed review.tech Awaiting Technical Review review.await-update Awaiting Updates after Edits labels May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review.copy Awaiting Copy Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite mixins page
4 participants