Skip to content

[cross_file] adopt code excerpts in README #5347

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.

Sign up for GitHub

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

Jump to bottom
Merged
auto-submit merged 10 commits into from
Nov 22, 2023
Merged

[cross_file] adopt code excerpts in README #5347

auto-submit merged 10 commits into from
Nov 22, 2023

Conversation

@mike-v2
Copy link
Contributor

@mike-v2 mike-v2 commented Nov 7, 2023 •
edited
Loading

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

Part of flutter/flutter#102679

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style].
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@mike-v2 mike-v2 requested a review from ditman as a code owner November 7, 2023 17:48
@mike-v2
Copy link
Contributor Author

mike-v2 commented Nov 7, 2023

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
ditman previously requested changes Nov 8, 2023
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

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)

@mike-v2
Copy link
Contributor Author

mike-v2 commented Nov 10, 2023

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
Copy link
Member

ditman commented Nov 11, 2023

(Force-pushed to rebase with latest main)

@ditman
Copy link
Member

ditman commented Nov 11, 2023

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!

@ditman ditman self-requested a review November 11, 2023 01:23
@ditman ditman dismissed their stale review November 11, 2023 01:28

Changes done, trying again.

@ditman
Copy link
Member

ditman commented Nov 11, 2023 •
edited
Loading

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 ditman force-pushed the code-excerpts-cross-file branch from 15c5c16 to 30463d9 Compare November 11, 2023 02:08
@ditman
Copy link
Member

ditman commented Nov 14, 2023

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" :)

@ditman ditman force-pushed the code-excerpts-cross-file branch from 038434c to 1aa314f Compare November 14, 2023 02:30
@mike-v2
Copy link
Contributor Author

mike-v2 commented Nov 14, 2023

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
Copy link
Member

ditman commented Nov 14, 2023 •
edited
Loading

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
Copy link
Contributor Author

mike-v2 commented Nov 14, 2023

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
Copy link
Member

ditman commented Nov 14, 2023 •
edited
Loading

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
ditman approved these changes Nov 15, 2023
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

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
stuartmorgan-g approved these changes Nov 19, 2023
View reviewed changes
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g 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 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!

@ditman ditman force-pushed the code-excerpts-cross-file branch from 4f1793d to c8f265d Compare November 21, 2023 12:18
@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 21, 2023

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.

@tarrinneal tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 22, 2023

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.

@tarrinneal tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 22, 2023

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.

@tarrinneal tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023
@auto-submit auto-submit bot merged commit 2102327 into flutter:main Nov 22, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 22, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 22, 2023
@engine-flutter-autoroll
2102327 [cross_file] adopt code excerpts in README (flutter/packages#…
e1bf29b 
…5347)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 22, 2023
@engine-flutter-autoroll
Roll Packages from c9933fc to 2102327 (3 revisions) (#138895)
721ee57 
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
Copy link
Member

ditman commented Nov 22, 2023 •
edited
Loading

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

caseycrogers pushed a commit to caseycrogers/flutter that referenced this pull request Dec 29, 2023
@engine-flutter-autoroll @caseycrogers
Roll Packages from c9933fc to 2102327 (3 revisions) (flutter#138895)
f9c0659 
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
@reidbaker reidbaker mentioned this pull request Jan 12, 2024
Closed
@reidbaker reidbaker mentioned this pull request Jan 19, 2024
Merged
12 tasks
@reidbaker reidbaker mentioned this pull request Feb 19, 2024
Merged
16 tasks
@jmagman jmagman mentioned this pull request Oct 9, 2025
Open
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ditman ditman ditman approved these changes

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: cross_file

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mike-v2 @ditman @stuartmorgan-g @tarrinneal