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

Fix factory constructor example #5889

Merged
merged 4 commits into from
Jun 6, 2024
Merged

Conversation

atsansone
Copy link
Contributor

@atsansone atsansone commented Jun 3, 2024

Fixes #5615
Fixes #5616

@atsansone atsansone added the review.tech Awaiting Technical Review label Jun 3, 2024
@atsansone atsansone requested a review from khanhnwin June 3, 2024 18:00
@dart-github-bot
Copy link
Collaborator

dart-github-bot commented Jun 3, 2024

Visit the preview URL for this PR (updated for commit 8120829):

https://dart-dev--pr5889-fix-5616-97ymunmk.web.app

```dart
// Implement this factory constructor.
factory IntegerHolder.fromList(List<int> list) {
if (list.length == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a switch statement?


if (obj is! IntegerSingle) {
errs.add('Called IntegerHolder.fromList([1]) and got an object of type \n ${obj.runtimeType} instead of IntegerSingle.');
switch (count) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might be able to use pattern matching here

} catch (e) {
print('Called IntegerSingle.fromList([]) and got an exception of \n type ${e.runtimeType}.');
return;
if (!testBadNumberOfArgs(errs, 0)) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not quite clear to me what this code is doing. Can I have a brief description?

} catch (e) {
print('Called IntegerHolder.fromList([1, 2]) and got an exception \n of type ${e.runtimeType}.');
return;
testValueOfA(
Copy link
Contributor

Choose a reason for hiding this comment

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

What does A, ABC, and X represent?


bool testBadNumberOfArgs(List<String> errs, int count) {
assert(count == 0 || count == 4);
Copy link
Contributor

Choose a reason for hiding this comment

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

should this check be in a separate function that is to test the value as opposed to the test itself?


bool testGoodNumberOfArgs(List<String> errs, int count) {
assert(1 <= count && count <= 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here with this assert


// Tests your solution (Don't edit!):
// Tests your solution (Don't edit from this point to end of file):
Copy link
Contributor

Choose a reason for hiding this comment

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

My primary concern here is that these tests are now more difficult to follow than they were previously in the context of a cheatsheet. Can it be simplified a bit more? @johnpryan @parlough can you double check me on that?

@atsansone atsansone added st.RFM Ready to merge or land and removed review.tech Awaiting Technical Review labels Jun 6, 2024
@atsansone atsansone merged commit 8574066 into dart-lang:main Jun 6, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
st.RFM Ready to merge or land
Projects
None yet
3 participants