Skip to content

Conversation

@brianegan
Copy link
Contributor

This PR adds a Dartpad workshop for Box Decoration and Text styling. This is a basic / introductory workshop for folks who are new to Flutter. It is the first workshop in a series that I'm writing for the Flutter team, and the current plan is to contribute all of my workshops to this repository so they can be properly maintained by the team after my contract ends.

I've already discussed with Nilay and John, but happy to discuss further if needed!

Pre-launch Checklist

  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-devrel channel on Discord.

@domesticmouse
Copy link
Contributor

You'll need to make some fixes to make the CI happy:

== Testing 'dartpad_codelabs'
Analyzing dartpad_codelabs...

  error - src/box_and_text_decoration/text/snippet.dart:11:10 - The body might complete normally, causing 'null' to be returned, but the return type is a potentially non-nullable type. Try adding either a return or a throw statement at the end. - body_might_complete_normally

@@ -0,0 +1,14 @@
// ignore_for_file: prefer_const_constructors, prefer_const_literals_to_create_immutables, use_key_in_widget_constructors
Copy link
Contributor

Choose a reason for hiding this comment

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

@redbrogdon should we make these default for DartPad codelab content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thoughts: It would be nice for the each workshop to have an analysis_options.yaml that could be customized.

For example, this workshop is aimed at true Flutter beginners. The first thing they learn about Flutter should be fun and stress-free. This workshop should help build confidence. It should focus on Flutter basics. The workshop shouldn't aim to teach newcomers how to optimize their build methods with const, it's just too early for that!

Therefore, it made sense to disable any const warnings or errors for this target audience. In more advanced workshops aimed at more experienced developers, const might be expected. That leads me to think it might be nice for each workshop to have some control over the analysis_options if possible.

What do think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@johnpryan thoughts on re-organising how DartPad codelabs so each experience can have it's own lint configuration?

Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out, this would require some fairly major modifications to DartPad's back end analysis service. We'd need to reboot the analyzer between different configurations, and that's waay too slow. We have to use one configuration for all users of DartPad

Copy link
Contributor Author

@brianegan brianegan Feb 1, 2022

Choose a reason for hiding this comment

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

Ah, I gotcha. Yes, that sounds very slow :) Thanks for taking a look at the issue! I'll continue to use inline comments where needed.

@@ -0,0 +1,17 @@
// ignore_for_file: prefer_const_constructors, use_key_in_widget_constructors
Copy link
Contributor

Choose a reason for hiding this comment

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

Please bring this PR up to date and delete these lines. They should no longer be needed.

Copy link
Contributor Author

@brianegan brianegan Jan 31, 2022

Choose a reason for hiding this comment

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

Thanks @domesticmouse! I've just updated my local branch, but when I preview the workshop in my browser I still receive const errors if I removed these lines. I've disabled my browser cache on Dartpad.dev, so I should have the latest code changes.

Do you have an idea of what might be happening here?

Screen Shot 2022-01-31 at 2 07 56 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @domesticmouse! I've just updated my local branch, but when I preview the workshop in my browser I still receive const errors if I removed these lines. I've disabled my browser cache on Dartpad.dev, so I should have the latest code changes.

Do you have an idea of what might be happening here?

Oh, I understand why you had those comments in place. DartPad has it's own lint rules.

I'll revert #273

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I'm going to align the lints on this directory with the rules enforced by DartPad itself. See #276

Copy link
Contributor

Choose a reason for hiding this comment

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

Please bring this PR up to date with master. The dartpad_codelab analysis rules are the same as DartPad.

Copy link
Contributor Author

@brianegan brianegan Feb 1, 2022

Choose a reason for hiding this comment

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

Hi @domesticmouse -- Hm, it looks like the PR you linked has not changed anything from my side. End users still receive the exact same warnings & errors as seen in the screenshot above.

Maybe we should take a step back? I kinda feel like we're going back and forth with PRs, but I'm not sure we're trying to achieve the same goal.

Overall, I would like to disable a few lints so they do not display any errors or warnings to users taking this workshop. I want to disable these warnings/errors because I do not think they are helpful for beginners. I want beginners to focus on BoxDecoration, not get sidetracked by const info messages. Therefore, I want to disable the following analysis options for this workshop:

- unused_import
- prefer_const_constructors
- prefer_const_literals_to_create_immutables
- use_key_in_widget_constructors

However, it does not look like either PR linked accomplishes the goal I'm trying to achieve, and I still need to include the ignores in each file.

Sorry, I hope I'm not missing anything! I don't have too much context on this project or how it works, so I'm not exactly sure where those analysis_options.yaml updates you've linked to are applied or how they're even used (only for CI checks, or are they applied for end-users as well?). However, they do not seem to change anything for end-users, which is who I'm targeting with my // ignore_for_file comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, you will need your ignore for file comments. I can't do individualised lints on DartPad IIUC.

@brianegan brianegan merged commit f78c0e4 into flutter:master Feb 3, 2022
@brianegan brianegan deleted the workshop-box-text-decoration branch February 3, 2022 18:23
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