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

#2559. Add augmented expression tests for setters #2618

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

sgrekhov
Copy link
Contributor

No description provided.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

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

Looks good! Just one thing: LanguageFeatures/Augmentation-libraries/augmented_expression_A02_t06.dart seems to imply that augmented = e in an augmentation will invoke a top-level setter in scope; it should actually invoke the original declaration (which will in turn invoke that top-level setter named augmented=). I'm suggesting that the log is adjusted a bit such that we can see that the invocation goes via the original declaration, and the description is adjusted to reflect this behavior.

String _log = "";

void set augmented(String value) {
throw Exception("Wrong setter is called");
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this one more time, it's not obvious which is nicer for a developer who is encountering some test failures. The approach where line 26 assigns a value to _log which is not expected in some Expect statement is perhaps more consistent: We get to see what went wrong in both cases, but having a wrong _log value is more like several other buggy behaviors.

Anyway, I don't see a reason to change anything here now.

@sgrekhov
Copy link
Contributor Author

Updated. Please take another look

@sgrekhov sgrekhov requested a review from eernstg April 24, 2024 10:42
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

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

LGTM. I left one thread open because I wanted to comment on it, but I don't think it's a very good idea to make changes because of that, so I'll just land this PR now.

String _log = "";

void set augmented(String value) {
throw Exception("Wrong setter is called");
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this one more time, it's not obvious which is nicer for a developer who is encountering some test failures. The approach where line 26 assigns a value to _log which is not expected in some Expect statement is perhaps more consistent: We get to see what went wrong in both cases, but having a wrong _log value is more like several other buggy behaviors.

Anyway, I don't see a reason to change anything here now.

@eernstg eernstg merged commit e6fdb61 into dart-lang:master Apr 24, 2024
1 check passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Apr 26, 2024
2024-04-26 sgrekhov22@gmail.com Fixes dart-lang/co19#2622. Fix typo, add issue numbers (dart-lang/co19#2623)
2024-04-25 sgrekhov22@gmail.com dart-lang/co19#2559. Add more augmenting expression tests for setters (dart-lang/co19#2620)
2024-04-24 sgrekhov22@gmail.com dart-lang/co19#2559. Add more augmenting expression tests for getters (dart-lang/co19#2619)
2024-04-24 sgrekhov22@gmail.com dart-lang/co19#2559. Add augmented expression tests for setters (dart-lang/co19#2618)
2024-04-24 sgrekhov22@gmail.com dart-lang/co19#2559. Add augmented expression tests for getters (dart-lang/co19#2617)
2024-04-23 sgrekhov22@gmail.com dart-lang/co19#2559. Fix augmenting operators syntax (dart-lang/co19#2616)
2024-04-22 sgrekhov22@gmail.com dart-lang/co19#2559. Add more augmenting functions tests (dart-lang/co19#2615)
2024-04-19 sgrekhov22@gmail.com dart-lang/co19#2559. Add augmenting functions signature tests. Part 2 (dart-lang/co19#2614)

Change-Id: I8c187cda88baa2e638f189e2a9251c479d14ad79
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/364502
Reviewed-by: Erik Ernst <eernst@google.com>
Commit-Queue: Erik Ernst <eernst@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
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.

None yet

2 participants