[tool] Add support for `.java`, `.gradle`, `.sh`, and `.m` files… #5567

mike-v2 · 2023-12-04T22:55:03Z

… to update-excerpts tool

Need this tool to recognize .java, .gradle, and .sh files in order to complete flutter/flutter#102679. The README.md for the espresso package contains code from .java, .gradle, and .sh files.

Support for .m files was requested by @tarrinneal.

Pre-launch Checklist

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

…update-excerpts` tool

flutter-dashboard · 2023-12-04T22:55:06Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

mike-v2 · 2023-12-04T23:09:16Z

The existing test file update_excerpts_command_test.dart only uses dart language in its tests. It seems like it would have to be completely reworked in order to allow testing other languages.

stuartmorgan-g · 2023-12-05T16:07:28Z

The existing test file update_excerpts_command_test.dart only uses dart language in its tests. It seems like it would have to be completely reworked in order to allow testing other languages.

What do you mean by "totally reworked"? It looks like the existing test structure would just need one more parameter (for the file path to write source to) in its helper.

Update test helper function to accept fileName parameter so that tests may use arbitrary file types Remove extra line in tool file that prevents language from being properly set in cc files

mike-v2 · 2023-12-05T17:50:12Z

Ah sorry, I should've figured that out. Thanks for pointing me in the right direction!

Group file type tests

mike-v2 · 2023-12-06T04:38:50Z

script/tool/test/update_excerpts_command_test.dart

+      });
+    }
+
+    // ignore: prefer_foreach


Two conflicting lint rules here: avoid_function_literals_in_foreach_calls and prefer_foreach. The latter aims to make the code more terse but the formatter uses the same number of lines with forEach as it does with for...in.

There's not a conflict, the linter just gives a misleading suggestion for what you probably wrote as your other version. The version of this that's compliant with all of our lints is testCases.forEach(runTest);.

stuartmorgan-g · 2023-12-12T21:07:18Z

script/tool/lib/src/update_excerpts_command.dart

@@ -142,10 +142,15 @@ class UpdateExcerptsCommand extends PackageLoopingCommand {
                  case '.kt':
                    language = 'kotlin';
                    break;
-                  case '.cc':


Why was this removed?

Shoot, there was a problem with my test case for .cc files. My mistake.

stuartmorgan-g · 2023-12-12T21:09:48Z

script/tool/test/update_excerpts_command_test.dart

@@ -33,12 +33,13 @@ void runAllTests(MockPlatform platform) {
      ));
  });

-  Future<void> testInjection(String before, String source, String after,
+  Future<void> testInjection(
+      String before, String source, String after, String fileName,


This is a lot of positional arguments with the same type; ideally they should all be named, but lets at least name the new one.

stuartmorgan-g · 2023-12-12T21:11:19Z

script/tool/test/update_excerpts_command_test.dart

@@ -33,12 +33,13 @@ void runAllTests(MockPlatform platform) {
      ));
  });

-  Future<void> testInjection(String before, String source, String after,
+  Future<void> testInjection(
+      String before, String source, String after, String fileName,


Nit throughout the PR: "filename" is generally a single word in programming, so the variable would just be filename.

stuartmorgan-g · 2023-12-12T21:11:32Z

script/tool/test/update_excerpts_command_test.dart

+      });
+    }
+
+    // ignore: prefer_foreach


There's not a conflict, the linter just gives a misleading suggestion for what you probably wrote as your other version. The version of this that's compliant with all of our lints is testCases.forEach(runTest);.

stuartmorgan-g

LGTM!

tarrinneal

Do you think there's a good way to handle .h files for c++ and objc?

tarrinneal · 2023-12-13T20:35:24Z

script/tool/lib/src/update_excerpts_command.dart

                  case '.cpp':
                    language = 'c++';
                    break;
+                  case '.m':
+                    language = 'objectivec';
+                    break;


@stuartmorgan Seems like we don't have support for header files for either language. I'm not sure there's a simple solution though.

Good point. That would need either more special casing here, or a more generalized solution (like a mapping from extensions to the set of allowed languages).

tarrinneal · 2023-12-13T20:37:32Z

script/tool/test/update_excerpts_command_test.dart

+      required String source,
+      required String after,
+      required String filename,
+      bool failOnChange = false}) async {


nit: add a trailing comma false,})

tarrinneal

Changed my mind, merging now

…les… (flutter/packages#5567)

flutter/packages@b5958e2...1151191 2023-12-14 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.8 to 3.22.11 (flutter/packages#5674) 2023-12-13 [email protected] [ci][web] Ignore always_specify_types for JSArray. (flutter/packages#5669) 2023-12-13 [email protected] [tool] Add support for `.java`, `.gradle`, `.sh`, and `.m` filesâ�¦ (flutter/packages#5567) 2023-12-13 [email protected] [google_sign_in] Update (web) example app. (flutter/packages#5634) 2023-12-13 [email protected] [path_provider] De-flake getExternalStorageDirectories test (flutter/packages#5628) 2023-12-13 [email protected] [google_sign_in_web] Stop relying on framework internals. (flutter/packages#5660) 2023-12-13 [email protected] [Android] Bump Gradle version to 7.6.3 (flutter/packages#5522) 2023-12-13 [email protected] [google_sign_in] Adopt code excerpts in README (flutter/packages#5521) 2023-12-13 [email protected] [css_colors] Adopt code excerpts in README (flutter/packages#5478) 2023-12-13 [email protected] [webview_flutter] Support for handling basic authentication requests (Android) (flutter/packages#5454) 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 added 2 commits December 4, 2023 14:31

[tool] Add support for .java, .gradle, .sh, and .m files to `…

8af458d

…update-excerpts` tool

Fix 'gradle' should not be listed as a language

576fc7b

Add tests to ensure different file types are updated properly

83a76b7

Update test helper function to accept fileName parameter so that tests may use arbitrary file types Remove extra line in tool file that prevents language from being properly set in cc files

Run tests in a 'for' loop

c13043e

Group file type tests

mike-v2 commented Dec 6, 2023

View reviewed changes

tarrinneal requested review from stuartmorgan-g and tarrinneal and removed request for stuartmorgan-g December 6, 2023 09:18

stuartmorgan-g requested changes Dec 12, 2023

View reviewed changes

mike-v2 added 5 commits December 12, 2023 14:53

Re-add recognition of '.cc' files as 'c++' language

bc628be

Replace 'fileName' with 'filename'

2a0a18a

Fix test case for extracting excerpts from '.cc' files

0ec0bd6

Convert 'testInjection' function arguments to named arguments

9c5ff63

Change 'for...in' loop to 'forEach'

dde5ebf

tarrinneal requested a review from stuartmorgan-g December 13, 2023 20:24

stuartmorgan-g approved these changes Dec 13, 2023

View reviewed changes

tarrinneal requested changes Dec 13, 2023

View reviewed changes

tarrinneal approved these changes Dec 13, 2023

View reviewed changes

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

auto-submit bot merged commit ee593fd into flutter:main Dec 13, 2023

engine-flutter-autoroll mentioned this pull request Dec 14, 2023

Roll Packages from b5958e2a8b49 to 1151191c8c13 (10 revisions) flutter/flutter#140154

Merged

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

ee593fd [tool] Add support for .java, .gradle, .sh, and .m fi…

633746a

…les… (flutter/packages#5567)

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

[tool] Add support for .java, .gradle, .sh, and .m files… #5567

[tool] Add support for .java, .gradle, .sh, and .m files… #5567

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Dec 4, 2023

Uh oh!

mike-v2 commented Dec 4, 2023

Uh oh!

stuartmorgan-g commented Dec 5, 2023

Uh oh!

mike-v2 commented Dec 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

tarrinneal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarrinneal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[tool] Add support for `.java`, `.gradle`, `.sh`, and `.m` files… #5567

[tool] Add support for `.java`, `.gradle`, `.sh`, and `.m` files… #5567

mike-v2 commented Dec 4, 2023 •

edited

Loading

stuartmorgan-g Dec 13, 2023 •

edited

Loading