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

adds null safety codelab #2983

Merged
merged 82 commits into from
Mar 22, 2021
Merged

adds null safety codelab #2983

merged 82 commits into from
Mar 22, 2021

Conversation

ryanmcginnis
Copy link
Contributor

@ryanmcginnis ryanmcginnis commented Feb 22, 2021

  • original snippets by @RedBrogdon/leaf
  • adds analysis_options.yaml and pubspec.yaml based on existing examples
  • adds examples to null_safety_codelab/bin/
  • adds codelab to codelabs/
  • updates side-nav.yml

staging: https://rm-dart.firebaseapp.com/codelabs/null-safety

@google-cla google-cla bot added the cla: yes Contributor has signed the Contributor License Agreement label Feb 22, 2021
@ryanmcginnis ryanmcginnis changed the title adds examples for null safety codelab adds null safety codelab Feb 22, 2021
@ryanmcginnis
Copy link
Contributor Author

(CC @RedBrogdon)

Copy link
Contributor

@filiph filiph left a comment

Choose a reason for hiding this comment

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

LGTM with some comments

src/codelabs/null-safety.md Outdated Show resolved Hide resolved
src/codelabs/null-safety.md Outdated Show resolved Hide resolved
src/codelabs/null-safety.md Outdated Show resolved Hide resolved
src/codelabs/null-safety.md Outdated Show resolved Hide resolved
src/codelabs/null-safety.md Outdated Show resolved Hide resolved
@ryanmcginnis
Copy link
Contributor Author

I think you missed some of my review comments. (I started marking them, but then... it seemed silly to point them all out.) I saw them at https://github.com/dart-lang/site-www/pull/2983/files/a2236dac7a7adda644902a423252bfc18c01473f..6f5f3aad7cc30a9701ba929bb4f84906c8ee7a53.

thanks. i remember clicking the commit button for each suggestion. but i see now that i need to wait for the page to reload each time.

@ryanmcginnis ryanmcginnis dismissed mit-mit’s stale review March 17, 2021 13:43

mit said that i could skip a second review from him.

Copy link
Contributor

@kwalrath kwalrath left a comment

Choose a reason for hiding this comment

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

I noticed a few more things, but I need to take another look with fresh eyes... tomorrow, I hope!

One more thing that I didn't notice until just now: the line between examples and exercises doesn't seem clear in this codelab. I think of examples as being things you can just run without modification, but I think every one of these DartPads except for final one ("Exercise: Late and lazy") requires some code changes.

Maybe they should all be examples? All except the last one exercises? Or maybe we should add a suggestion for how they could modify the last one?

In https://dart.dev/codelabs/async-await, exercises have a dark background and examples have a white background, but since the changes required here are so minor, differentiating the backgrounds seems perhaps unnecessary.

src/codelabs/null-safety.md Outdated Show resolved Hide resolved
src/codelabs/null-safety.md Outdated Show resolved Hide resolved
src/codelabs/null-safety.md Outdated Show resolved Hide resolved
src/codelabs/null-safety.md Outdated Show resolved Hide resolved
src/codelabs/null-safety.md Outdated Show resolved Hide resolved
src/codelabs/null-safety.md Outdated Show resolved Hide resolved
src/codelabs/null-safety.md Outdated Show resolved Hide resolved
src/codelabs/null-safety.md Outdated Show resolved Hide resolved
src/codelabs/null-safety.md Outdated Show resolved Hide resolved
src/codelabs/null-safety.md Outdated Show resolved Hide resolved
Copy link
Contributor

@kwalrath kwalrath left a comment

Choose a reason for hiding this comment

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

I think we're ready to go. I'll file a bug for follow-up work, like adding this to the /codelabs page and perhaps adding hints & tests to the examples.

Copy link
Member

@parlough parlough 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 overall, but some minor comments that can be implemented in a followup PR.

Comment on lines +2 to +4
List<String> aListofStrings = ['one', 'two', 'three'];
List<String?> aNullableListOfStrings = [];
List<String?> aListofNullableStrings = ['one', null, 'three'];
Copy link
Member

Choose a reason for hiding this comment

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

Seems like of should be Of to be consistent with the middle declaration and general naming standards.

Comment on lines +2 to +4
late String description;

void setDescription(String str) {
Copy link
Member

Choose a reason for hiding this comment

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

It seems very not "Dart" like to include a setter method like this. Perhaps you could switch this example to use a getter and setter:

Late variables aren't generally a good idea to expose publicly anyway as you don't have much control over what others will do.

class Meal {
  late String _description;

  void set description(String str) => _description = str;

  String get description => _description;
}

void main() {
  final myMeal = Meal();
  myMeal.description = 'Feijoada!';
  print(myMeal.description);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Goodness, this sounds reasonable but we might need to think about it. I'll add it to that follow-up issue.

src/codelabs/null-safety.md Outdated Show resolved Hide resolved
Co-authored-by: Parker Lougheed <[email protected]>
@kwalrath kwalrath merged commit 63206b9 into master Mar 22, 2021
@kwalrath kwalrath deleted the rrm-null-safety branch March 22, 2021 23:47
@kwalrath kwalrath mentioned this pull request Mar 22, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Contributor has signed the Contributor License Agreement p1-high Major but not urgent concern: Resolve in months. Update each month.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants