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 more augmenting functions tests #2615

Merged
merged 3 commits into from
Apr 22, 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, with a typo or two!

Just one proposal goes a bit further: It looks like LanguageFeatures/Augmentation-libraries/augmenting_functions_A05_t02.dart gives rise to some error messages that are expected (so they are treated correctly), but will also potentially cause some confusion.

@sgrekhov
Copy link
Contributor Author

Updated. Please teke another look

@sgrekhov sgrekhov requested a review from eernstg April 22, 2024 12:57
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!

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.

One more thing came up.


augment library 'augmenting_functions_A06_t02.dart';

augment external void topLevelFunction() {
Copy link
Member

Choose a reason for hiding this comment

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

One more thing! The grammar doesn't allow augment external.

I think it's supposed to be written like this:

Suggested change
augment external void topLevelFunction() {
augment void topLevelFunction() {

and it is not intended to be an error that the augmenting declaration doesn't have external.

In that sense, external plays a role which is similar to a body (so the original and the augmenting declarations can differ), not a modifier (where the original and augmenting declarations must be identical).

Copy link
Member

Choose a reason for hiding this comment

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

@sgrekhov
Copy link
Contributor Author

Thank you for noticing! Fixed

@sgrekhov sgrekhov requested a review from eernstg April 22, 2024 13:58
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.

Thanks! LGTM.

@eernstg eernstg merged commit 1999ed9 into dart-lang:master Apr 22, 2024
2 checks 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