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 augmenting declarations tests #2565

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

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!

LanguageFeatures/Augmentation-libraries/merge_order_A01_t01.dart wouldn't be testable for a while because augmented isn't likely to be implemented during the next couple of months.

But the merging order could be tested without augmented by having the same graph (main plus lib1, lib2, lib3) and letting main introduce class A, lib1 augment class A and introduce class B, lib2 augment class B and introduce class C, and finally lib3 augment class C.

This could be done without using augmented anywhere, and it would test the ordering.


// SharedOptions=--enable-experiment=macros

augment class C {}
Copy link
Member

Choose a reason for hiding this comment

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

If the language team (1) accepts the proposal that I hinted at in dart-lang/language#3647, or (2) tells me that we already have such a rule then this would be an error simply because it isn't in an augmentation library.

So we ought to have a test where it does occur in an augmentation library, and still there is no original declaration to augment.

Copy link
Member

Choose a reason for hiding this comment

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

(This might be covered by tests further down, I haven't looked at them all).


import augment 'augmenting_declarations_A02_t01_lib.dart';

/**/augment class C {}
Copy link
Member

Choose a reason for hiding this comment

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

If they accept #3647 then this would be a different error, in which case the description would need to be adjusted.

@sgrekhov sgrekhov changed the title #2559. Add merge order and augmenting declarations tests #2559. Add augmenting declarations tests Mar 12, 2024
@sgrekhov sgrekhov added the status-blocked Blocked from making progress by another (referenced) issue label Mar 12, 2024
@sgrekhov
Copy link
Contributor Author

Merge order tests updated and moved to #2566. This one is blocked by dart-lang/language#3647 for now

@eernstg
Copy link
Member

eernstg commented Mar 16, 2024

This one is blocked by dart-lang/language#3647 for now

I'm not sure we have a clear policy, but perhaps such PRs usually have a 'Draft' status?

@sgrekhov sgrekhov marked this pull request as draft March 25, 2024 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status-blocked Blocked from making progress by another (referenced) issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants