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

#2142. Add syntax tests #2148

Merged
merged 2 commits into from
Jul 25, 2023
Merged

#2142. Add syntax tests #2148

merged 2 commits into from
Jul 25, 2023

Conversation

sgrekhov
Copy link
Contributor

It turns out that it's easier to add new etxtension types tests (which are, in fact, edited copies of inline classes tests) rather then rename and change inline classes ones. I'm going to go on this way and when there will be nothing more to copy and edit I'll delete Inline classes folder

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 I believe must be changed: ET.new(42) is not an error when ET declares a constructor (a primary constructor in this case, but any constructor would do) whose name is ET.

I also mentioned a couple of places where a test will need to be adjusted in the future if we adopt the primary constructor proposal (in its current form or something that isn't too different). I don't know what's the best approach; perhaps we can just ignore this situation and rely on our ability to rediscover it when needed? .. another approach could be to put a TODO comment saying that "when we have primary constructors, this needs to change as follows: ...". WDYT?

Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

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

Updated. Added TODO for the tests that may be affected by the primary constructors

@sgrekhov sgrekhov requested a review from eernstg July 25, 2023 10:32
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 f500a37 into dart-lang:master Jul 25, 2023
1 of 2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jul 31, 2023
2023-07-28 sgrekhov22@gmail.com dart-lang/co19#2142. Fix typo (dart-lang/co19#2155)
2023-07-26 sgrekhov22@gmail.com dart-lang/co19#2149. Fix new errors due to changes with the constant evaluator (dart-lang/co19#2152)
2023-07-26 sgrekhov22@gmail.com dart-lang/co19#2145. Add more tests for Variable (dart-lang/co19#2146)
2023-07-25 sgrekhov22@gmail.com dart-lang/co19#2142. Add syntax tests (dart-lang/co19#2148)

Change-Id: I6ee222b74fc092d93a1b76d35d89a008a97056be
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/316590
Commit-Queue: William Hesse <whesse@google.com>
Reviewed-by: William Hesse <whesse@google.com>
@sgrekhov sgrekhov deleted the co19-2142-1 branch November 14, 2023 11:28
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.

2 participants