[cross_file] adopt code excerpts in README by mike-v2 · Pull Request #5347 · flutter/packages

mike-v2 · 2023-11-07T17:48:48Z

Updates the README to use a compiled excerpt source for its example of instantiating an XFile.

Pre-launch Checklist

mike-v2 · 2023-11-07T17:49:00Z

The test I made fails because the code in the excerpt file tries to load a text file from the cross_file/tests folder, but I'm not sure how to access that file from the cross_file/example/lib folder.

ditman

Hey @mike-v2, thanks for tackling this!

A couple of quick comments:

flutter analyze is complaining about a couple of things, here's the excerpt from the error logs:

   info - example/lib/readme_excerpts.dart:12:3 - Don't invoke 'print' in production code. Try using a logging framework. - avoid_print
   info - example/lib/readme_excerpts.dart:13:3 - Don't invoke 'print' in production code. Try using a logging framework. - avoid_print
   info - example/lib/readme_excerpts.dart:14:3 - Don't invoke 'print' in production code. Try using a logging framework. - avoid_print
   info - example/lib/readme_excerpts.dart:15:3 - Don't invoke 'print' in production code. Try using a logging framework. - avoid_print
   info - example/lib/readme_excerpts.dart:18:3 - Don't invoke 'print' in production code. Try using a logging framework. - avoid_print
   info - example/lib/readme_excerpts.dart:18:31 - Unnecessary braces in a string interpolation. Try removing the braces. - unnecessary_brace_in_string_interps

Also, there's a failure on the "web" test runner; the problem is that on the web path doesn't mean anything, so a (real) XFile on the web can only be created from Bytes.

I've suggested a couple of fixes to readme_excerpts.dart and its test that might allow this to pass (but these are "off the top of my head" suggestions; please do test locally if they work as intended :P)

packages/cross_file/example/lib/readme_excerpts.dart

packages/cross_file/example/pubspec.yaml

packages/cross_file/example/test/readme_excerpts_test.dart

mike-v2 · 2023-11-10T03:54:02Z

Hey sorry for the easy mistakes and thanks for the detailed reply, it really helps a noob like me!

I was having trouble with using 'TestOn' (it never seemed to work, even when I imported from package:test, and my IDE was telling me that it can only be used on libraries), so I used 'kIsWeb' from 'flutter/foundation'. I hope this is ok.
Also, I wasn't able to get my test to pass unless I copied the assets/hello.txt file to the cross_file/example folder. This doesn't seem like the most elegant solution but I couldn't figure out how to make my test pass without it.

ditman · 2023-11-11T01:11:55Z

(Force-pushed to rebase with latest main)

ditman · 2023-11-11T01:18:04Z

I was having trouble with using 'TestOn' (it never seemed to work, even when I imported from package:test, and my IDE was telling me that it can only be used on libraries), so I used 'kIsWeb' from 'flutter/foundation'. I hope this is ok.

Yes, however the solution is even simpler, you can add a library 'name_of_the_test'; at the top of the dart file, and add the TestOn annotation to it, and that should be enough (I will push the small change I did).

Also, I wasn't able to get my test to pass unless I copied the assets/hello.txt file to the cross_file/example folder. This doesn't seem like the most elegant solution but I couldn't figure out how to make my test pass without it.

This is normal, everything inside "example" is its own stand-alone app, and the assets for that app must be contained inside of it.

One last change I did was to remove the dependencies on "flutter" from the "example" since it seems we only need package:test to run the test, and not flutter_test or flutter for that matter. I'll push my changes and see how much I confuse CI :)

Thanks for your contribution!

Changes done, trying again.

ditman · 2023-11-11T01:33:12Z

This PR is not updating after my changes (similar to: https://stackoverflow.com/questions/45626986). I'll check again on monday, but if it doesn't work I might have to ask you to close and re-create this PR @mike-v2!

ditman · 2023-11-14T02:01:00Z

I think my TestOn change made CI very unhappy with "no tests running"

Running command: "dart run test --platform=chrome" in /b/s/w/ir/x/w/packages/packages/cross_file/example
No tests ran.
No tests were found.

[packages/cross_file/example completed in 0m 12s]

I guess I'll revert my change and go back to skip: kIsWeb so some test "runs" :)

mike-v2 · 2023-11-14T02:31:27Z

Oh that's what was causing the 5 failing checks? Interesting. Waiting for all those tests to go green is like watching the longest spin of the Price is Right wheel.
Anyway I definitely learned a lot, and I appreciate your help. I'll try to do some more of these when I get the time!

ditman · 2023-11-14T02:32:38Z

Oh that's what was causing the 5 failing checks?

@mike-v2 I think that was causing only the "web" tests to fail. The others were most likely flaky tests that had nothing to do with your change, in fact :)

Waiting for all those tests to go green is like watching the longest spin of the Price is Right wheel.

Do you have access to the test failure logs, by following links from Github into the Flutter dashboard?

Anyway I definitely learned a lot, and I appreciate your help. I'll try to do some more of these when I get the time!

Thanks!! And most importantly: Yes please, do keep the fixes coming!

mike-v2 · 2023-11-14T02:53:41Z

Yeah I have access to the failure logs, which is awesome because it allows me to fix many of those simple mistakes. That's why I had a few commits in a row awhile back - I kept watching it fail and then looking through the logs to find what went wrong. Definitely learned a lot by doing that!

ditman · 2023-11-14T21:51:38Z

I think this should be ready to land now. Adding @stuartmorgan as 2nd team reviewer, for when he has a while.

Stuart, in this PR, we're attempting to test the code of the file that is used to generate the excerpts, I think this is a 1st!

ditman

LGTM!

(It might be a little odd testing the file that is used to generate the excerpts, but it looks good to me! The weird part is not using integration_test in the example, but since cross_file is a dart package, I didn't want to add any flutter deps, and the CI seems to Just Work™)

stuartmorgan-g

LGTM with some nits. Thanks!

Stuart, in this PR, we're attempting to test the code of the file that is used to generate the excerpts, I think this is a 1st!

Not quite a first, but definitely something we aren't doing very much of yet. It's great to see more of it!

packages/cross_file/example/lib/readme_excerpts.dart

packages/cross_file/example/pubspec.yaml

packages/cross_file/example/test/readme_excerpts_test.dart

auto-submit · 2023-11-21T12:37:11Z

auto label is removed for flutter/packages/5347, due to - The status or check suite Linux repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

add .

auto-submit · 2023-11-22T00:05:22Z

auto label is removed for flutter/packages/5347, due to - The status or check suite Linux_android android_platform_tests_shard_4 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2023-11-22T01:05:56Z

auto label is removed for flutter/packages/5347, due to - The status or check suite Linux_android android_platform_tests_shard_4 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

…5347)

flutter/packages@c9933fc...2102327 2023-11-22 [email protected] [cross_file] adopt code excerpts in README (flutter/packages#5347) 2023-11-21 [email protected] Manual roll Flutter from 9c9e061 to ab721f9 (16 revisions) (flutter/packages#5461) 2023-11-21 [email protected] [url_launcher_web] migrate to pkg:web (flutter/packages#5451) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

ditman · 2023-11-22T20:32:14Z

This landed and got published, thanks @mike-v2 for the contribution, and @tarrinneal for keeping the PR alive :D!

flutter/packages@c9933fc...2102327 2023-11-22 [email protected] [cross_file] adopt code excerpts in README (flutter/packages#5347) 2023-11-21 [email protected] Manual roll Flutter from 9c9e061 to ab721f9 (16 revisions) (flutter/packages#5461) 2023-11-21 [email protected] [url_launcher_web] migrate to pkg:web (flutter/packages#5451) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

mike-v2 requested a review from ditman as a code owner November 7, 2023 17:48

github-actions bot added the p: cross_file label Nov 7, 2023

ditman previously requested changes Nov 8, 2023

View reviewed changes

ditman self-requested a review November 11, 2023 01:23

ditman force-pushed the code-excerpts-cross-file branch from 15c5c16 to 30463d9 Compare November 11, 2023 02:08

ditman force-pushed the code-excerpts-cross-file branch from 038434c to 1aa314f Compare November 14, 2023 02:30

ditman approved these changes Nov 15, 2023

View reviewed changes

tarrinneal requested a review from stuartmorgan-g November 16, 2023 23:03

stuartmorgan-g approved these changes Nov 19, 2023

View reviewed changes

mike-v2 and others added 7 commits November 21, 2023 04:14

[cross_file] adopt code excerpts in README

e412bfa

fix test skips web

a9dea62

update Flutter version to 3.7.0 in example pubspec

8b59087

update code excerpt in README

b293b96

update min Dart version to 2.19.0 in example pubspec

80ed7a1

Simplify deps.

fab106b

Remove flutter dep. Determine kIsWeb fromEnvironment.

c8f265d

ditman force-pushed the code-excerpts-cross-file branch from 4f1793d to c8f265d Compare November 21, 2023 12:18

Address PR comments.

b188fb4

ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2023

ditman and others added 2 commits November 21, 2023 04:56

Bump min sdk version in example.

6c13a0d

Update readme_excerpts.dart

9a020a5

add .

tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023

tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023

tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023

auto-submit bot merged commit 2102327 into flutter:main Nov 22, 2023

engine-flutter-autoroll mentioned this pull request Nov 22, 2023

Roll Packages from c9933fc09a0b to 2102327ab9f2 (3 revisions) flutter/flutter#138895

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 22, 2023

2102327 [cross_file] adopt code excerpts in README (flutter/packages#…

e1bf29b

…5347)

reidbaker mentioned this pull request Jan 12, 2024

Dialog Debug #5881

Closed

reidbaker mentioned this pull request Jan 19, 2024

[in_app_purchase] Add play country code api #5941

Merged

12 tasks

reidbaker mentioned this pull request Feb 19, 2024

[in_app_purchase_android] Add UserChoiceBilling mode. #6162

Merged

16 tasks

jmagman mentioned this pull request Oct 9, 2025

[packages] Adopt code-excerpt across all READMEs flutter/flutter#102679

Open

31 tasks

Conversation

mike-v2 commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

mike-v2 commented Nov 7, 2023

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mike-v2 commented Nov 10, 2023

Uh oh!

ditman commented Nov 11, 2023

Uh oh!

ditman commented Nov 11, 2023

Uh oh!

ditman commented Nov 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ditman commented Nov 14, 2023

Uh oh!

mike-v2 commented Nov 14, 2023

Uh oh!

ditman commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mike-v2 commented Nov 14, 2023

Uh oh!

ditman commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

auto-submit bot commented Nov 21, 2023

Uh oh!

auto-submit bot commented Nov 22, 2023

Uh oh!

auto-submit bot commented Nov 22, 2023

Uh oh!

ditman commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mike-v2 commented Nov 7, 2023 •

edited

Loading

ditman commented Nov 11, 2023 •

edited

Loading

ditman commented Nov 14, 2023 •

edited

Loading

ditman commented Nov 14, 2023 •

edited

Loading

ditman commented Nov 22, 2023 •

edited

Loading