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

#1400. [Views] Syntax tests added #1498

Merged
merged 5 commits into from
Oct 11, 2022
Merged

Conversation

sgrekhov
Copy link
Contributor

@sgrekhov sgrekhov commented Oct 7, 2022

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.

Please include constant constructors as well. They are not mentioned, and we're discussing possible syntaxes for specifying that the primary constructor is constant, but it would be nice to ensure that testing for them is already included such that we don't forget.

Looks good! A few things are marked as errors, but are actually allowed, and I marked a couple of typos, too.


library views_lib;

int x = 42;
Copy link
Member

Choose a reason for hiding this comment

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

Please add the newline at the end, you never know which (UNIX-ish) tools will be surprised when it's missing.

/// The token `view` is made a built-in identifier.
///
/// @description Checks that it is a compile-time error to declare a function
/// named `view`
Copy link
Member

Choose a reason for hiding this comment

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

No, that would actually not be an error. I've specified that view is a built-in identifier, which means that it would be an error to declare a type with that name, but other things (a function, a local variable, etc, anything-not-an-type) would be ok.

LanguageFeatures/Views/syntax_A01_t04.dart Show resolved Hide resolved
/// The token `view` is made a built-in identifier.
///
/// @description Checks that it is a compile-time error to declare a constant
/// named `view`
Copy link
Member

Choose a reason for hiding this comment

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

Again not an error.


// SharedOptions=--enable-experiment=extension-types

import "views_lib.dart" as view;
Copy link
Member

Choose a reason for hiding this comment

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

Yes! A prefix is not a type, but it is indeed included among the things that can't have a name which is a built-in identifier. I forgot to mention that above.

/// @description Checks that if a view declaration named View that does not
/// include a <viewPrimaryConstructor>, the name of the representation is the
/// name id of the unique final instance variable that it declares, and the type
/// of the representation is the declared type of id
Copy link
Member

Choose a reason for hiding this comment

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

Yes, in this case the syntax is different, but the representation name/type should be tested in the same way, statically and dynamically.

LanguageFeatures/Views/syntax_A06_t03.dart Outdated Show resolved Hide resolved
LanguageFeatures/Views/syntax_A07_t01.dart Outdated Show resolved Hide resolved
}

view class View2(int id) {
void set id(int val) {}
Copy link
Member

Choose a reason for hiding this comment

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

The name of this setter is actually id=, so that's not an error.

LanguageFeatures/Views/syntax_A07_t02.dart Show resolved Hide resolved
@sgrekhov sgrekhov requested a review from eernstg October 10, 2022 08:33
@sgrekhov
Copy link
Contributor Author

I added const constructors tests and did a fixes according to the other notes. Please rereview

Expect.isTrue(v1 is int);
Expect.isTrue(v2 is num);
Expect.isTrue(v2 is double);
}
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 missing newline at the end.

Expect.isTrue(v1 is int);
Expect.isTrue(v2 is num);
Expect.isTrue(v2 is double);
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing newline at the end.

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 a couple of missing newlines.

@sgrekhov
Copy link
Contributor Author

Thanx. Fixed. Please rereview

@sgrekhov sgrekhov requested a review from eernstg October 11, 2022 07:08
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

@eernstg eernstg merged commit 6254fa4 into dart-lang:master Oct 11, 2022
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Oct 24, 2022
2022-10-21 sgrekhov22@gmail.com Fixes dart-lang/co19#1511. [Records] Remove `records` experimental flag from legacy libraries (dart-lang/co19#1512)
2022-10-21 sgrekhov22@gmail.com Fixes dart-lang/co19#1477. [Records] Type inference tests updated (dart-lang/co19#1500)
2022-10-21 sgrekhov22@gmail.com Fixes dart-lang/co19#1505. [Records] Added tests for dynamic access to record fields (dart-lang/co19#1507)
2022-10-19 sgrekhov22@gmail.com dart-lang/co19#1399. [Records] Interaction with legacy tests added (dart-lang/co19#1502)
2022-10-19 sgrekhov22@gmail.com dart-lang/co19#195. Typos fixed (dart-lang/co19#1508)
2022-10-19 sgrekhov22@gmail.com dart-lang/co19#993. [ffi] nullptr test added (dart-lang/co19#1506)
2022-10-14 asashour@yahoo.com Fix typos (dart-lang/co19#1504)
2022-10-12 sgrekhov22@gmail.com Fixes dart-lang/co19#1427. Expando tests added (dart-lang/co19#1479)
2022-10-12 sgrekhov22@gmail.com dart-lang/co19#1399. [Records] More members tests added (dart-lang/co19#1501)
2022-10-12 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 3.0.2 to 3.1.0 (dart-lang/co19#1499)
2022-10-11 sgrekhov22@gmail.com dart-lang/co19#1400. [Views] Syntax tests added (dart-lang/co19#1498)
2022-10-07 sgrekhov22@gmail.com Fixes dart-lang/co19#1491. Use correct record type in 'as' expression (dart-lang/co19#1497)
2022-10-07 sgrekhov22@gmail.com Fixes dart-lang/co19#1489. Don't use '?' in expressions (dart-lang/co19#1495)
2022-10-07 sgrekhov22@gmail.com Fixes dart-lang/co19#1488. Use correct record types (dart-lang/co19#1493)
2022-10-07 sgrekhov22@gmail.com Fixes dart-lang/co19#1490. Missing parameters names added (dart-lang/co19#1496)

Change-Id: I6d26701ee14a0c7820e3c3094d7d850c0f914028
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/265087
Reviewed-by: Alexander Thomas <athom@google.com>
@sgrekhov sgrekhov deleted the co19-1400-1 branch December 26, 2022 15:27
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