License Utility #8460

mossmana · 2024-10-21T19:02:52Z

Created a utility for updating licenses in configured source files that can be run via tests and/or standalone.

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 the Flutter Style Guide recently, and have followed its advice.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or there is a reason for not adding tests.

If you need help, consider asking for help on Discord.

packages/devtools_shared/lib/src/utils/license_utils.dart

mossmana · 2024-10-22T17:32:57Z

@kenzieschmoll This is the first part of the work that just covers the license util and associated tests (repo wide check test currently skipped). I will need to quickly follow-up with a couple more PRs:

A small PR for the implementation of the standalone DevTool command that can bulk update the licenses in configured files
The extremely large PR after I have run the command on ~1025 files to update the licenses

kenzieschmoll

Done with first pass.

tool/lib/license_utils.dart

kenzieschmoll · 2024-10-22T18:41:44Z

tool/lib/license_utils.dart

+  YamlList removeLicenses = YamlList();
+  YamlList addLicenses = YamlList();
+  YamlList includePaths = YamlList();
+  YamlList excludePaths = YamlList();
+  YamlMap fileTypes = YamlMap();


let's make all of these final. Additionally, can we add dart doc?

If they're final, I won't be able to overwrite them in LicenseConfig.fromYamlFile.

These should probably be defined as late final Yaml{List,Map} variable then.

These can be final now that we are using a factory constructor.

tool/lib/license_utils.dart

kenzieschmoll · 2024-10-22T18:44:40Z

tool/lib/license_utils.dart

+    String storedName,
+    String existingLicenseText,
+    String replacementLicenseText,
+    String content,


nit: can we make these required named parameters for better readability at the call site?

I'm still learning Dart after 20+ years of Java/Kotlin (not much time for coding anymore). So thanks for bearing with me. Will it end up looking like _processHeaders(storedName: storedName, existingLicenseText: existingLicenseText...). For a private method that is only called internally, does it still make sense?

It can be a bit redundant when the variable names match the parameter names. This is a case where "flutter-like" style doesn't always create the most verbose code. For pure Dart code, this isn't necessarily a hard and fast rule, but more a judgement call for readability. Since the required named params would create redundancy here, I think it is fine either way so if you want to leave as positional params that is fine 👍

tool/lib/license_utils.dart

kenzieschmoll · 2024-10-22T19:19:08Z

tool/lib/license_utils.dart

+              existingHeader,
+              replacementHeader,
+            );
+            final File backupFile = file.copySync('${file.path}.bak');


what's the backupFile for? it doesn't look like we do anything with it.

This is in the event something bad happens and the changes need to be rolled back. (This utility may be run by someone who hasn't committed code changes yet, for example.)

We delete backupFile below though, don't we?

If there is an exception, the program aborts before getting to the deletion.

Maybe over architecting here, but wanted to have some fallback. Is there a better way to handle this case?

Do we expect to encounter exceptions? Do you have any examples of cases where we could hit them?

We'd probably be better off including a --dry-run to tell users which files are going to be modified without actually modifying them. If you want to be extra thorough, you could check to see if the tree is clean or not and ask for confirmation before running the tool.

I like the dry-run idea. I'll go with that and just leave it up to the end user to decide how to proceed. Thanks!

kenzieschmoll · 2024-10-22T19:20:36Z

tool/lib/license_utils.dart

+
+  /// Bulk update license headers for files in the [directory] as configured
+  /// in the [config]. Returns a list of file paths that were updated.
+  Future<Map<String, List<String>>> bulkUpdate(


does this need to be a method on the LicenseHeader class? or should it be a standalone method in the file? Another option is to make this a static method so that an instance of LicenseHeader does not need to be created in order to call this method.

It references class member functions such as getReplacementInfo and rewriteLicenseHeader.

bkonyi · 2024-10-22T20:50:54Z

tool/lib/license_utils.dart

+  /// [p.extension]).
+  String _removeDotFromExtension(String ext) {
+      if (ext.startsWith('.')) {
+        return ext.replaceFirst('.', '');


Nit: maybe use ext.substring(1) instead?

bkonyi · 2024-10-22T20:54:18Z

tool/lib/license_utils.dart

+  ) async {
+    final File rewrittenFile = File('${file.path}.tmp');
+    // TODO: [mossmana] Convert to using a stream
+    await file.readAsString().then((String contents) async {


Nit: avoid using .then(...) if the code can be written sequentially using await.

bkonyi · 2024-10-22T20:57:35Z

tool/lib/license_utils.dart

+              replacementHeader,
+            );
+            final File backupFile = file.copySync('${file.path}.bak');
+            if (await rewrittenFile.length() > 0) {


Consider using lengthSync() here instead.

bkonyi · 2024-10-22T21:03:07Z

tool/lib/license_utils.dart

+    await for (final content in stream) {
+      // Return just the license headers for the simple case with no stored
+      // value requested (i.e. content matches licenseText verbatim)
+      if (content.contains(existingLicenseText)) {


I think this could possibly misbehave if content is split in the middle of a line.

I think it might be better to read the entire file (or some fixed number of bytes from the beginning of the file) using file.readAsStringSync() and then perform these checks.

For large files, this could potentially cause performance slowdowns. I will keep an eye on this, but it seems to work okay for the files in our repo.

bkonyi · 2024-10-22T21:04:40Z

tool/lib/license_utils.dart

+  ) async {
+    final File rewrittenFile = File('${file.path}.tmp');
+    // TODO: [mossmana] Convert to using a stream
+    await file.readAsString().then((String contents) async {


Additionally, consider using readAsStringSync() instead. This tool isn't doing any parallel work so the asynchronous overhead of the standard dart:io calls will actually be slower than blocking until the synchronous call returns.

bkonyi · 2024-10-22T21:05:02Z

tool/lib/license_utils.dart

+        existingHeader,
+        replacementHeader,
+      );
+      await rewrittenFile.writeAsString(replacementContents);


Ditto about using the synchronous FS APIs.

kenzieschmoll · 2024-10-22T22:50:51Z

tool/lib/license_utils.dart

+  late final YamlList removeLicenses;
+
+  /// Sequence of license text strings that should be added to the top of a
+  /// file.
+  late final YamlList addLicenses;
+
+  /// Path(s) to recursively check for file to remove/add license text
+  late final YamlList includePaths;
+
+  /// Path(s) to recursively check for files to ignore
+  late final YamlList excludePaths;
+
+  /// Contains the extension (without a '.') and the associated indices
+  /// of [removeLicenses] to remove and index of [addLicenses] to add for the
+  /// file type.
+  late final YamlMap fileTypes;


these no longer need to be late now that LicenseConfig.fromYamlFile is a factory constructor.

also, style-ordering-nit: order as follows:
// constructor
// factory constructors
// final fields

tool/lib/license_utils.dart

mossmana · 2024-10-23T18:58:54Z

@kenzieschmoll @bkonyi I think I addressed the majority of the feedback. So, probably in a good state for a follow-up review when you get the chance. Please let me know if I missed anything.

tool/lib/license_utils.dart

kenzieschmoll · 2024-10-23T20:28:58Z

I went through and resolved open comments where possible. There are still a few open comments to address, but looking good!

bkonyi · 2024-10-24T14:50:58Z

tool/lib/license_utils.dart

+    required String replacementHeader,
+  }) {
+    final rewrittenFile = File('${file.path}.tmp');
+    // TODO(mossmana): Convert to using a stream


Is this TODO relevant anymore?

bkonyi · 2024-10-24T14:53:39Z

tool/lib/license_utils.dart

+  ///
+  /// If [dryRun] is set, returns a list of file paths that should be updated,
+  /// but no files will be actually be updated.
+  Future<Map<String, List<String>>> bulkUpdate({


It's not clear just by looking at this return type and reading the documentation what the format of this returned map is. Consider making the documentation more explicit or make use of typedefs to make it more clear what the keys and values in the map represent.

@bkonyi I switched to a typedef and tried to make the docs a bit clearer. Let me know if there's anything else I should add.

…n replacement header, and added more tests

…st on CI

…dback

mossmana added the release-notes-not-required label Oct 21, 2024

mossmana changed the title ~~Issue8216~~ License Utility Oct 21, 2024

kenzieschmoll reviewed Oct 21, 2024

View reviewed changes

packages/devtools_shared/lib/src/utils/license_utils.dart Outdated Show resolved Hide resolved

mossmana force-pushed the issue8216 branch from 39df4d0 to 1743ff5 Compare October 22, 2024 15:51

mossmana marked this pull request as ready for review October 22, 2024 17:25

mossmana requested review from bkonyi and a team as code owners October 22, 2024 17:25

kenzieschmoll reviewed Oct 22, 2024

View reviewed changes

bkonyi reviewed Oct 22, 2024

View reviewed changes

kenzieschmoll reviewed Oct 22, 2024

View reviewed changes

tool/lib/license_utils.dart Outdated Show resolved Hide resolved

kenzieschmoll reviewed Oct 23, 2024

View reviewed changes

tool/lib/license_utils.dart Show resolved Hide resolved

bkonyi reviewed Oct 24, 2024

View reviewed changes

mossmana requested a review from elliette as a code owner October 24, 2024 19:52

mossmana added 13 commits October 24, 2024 12:57

Initial YAML config parsing logic and tests

b0ed4c5

added additional YAML config parsing logic

98e9d3c

Added comments and some logic to parse stored value

20a449e

renamed functions to better match functionality, added logic to retur…

bd8e5c4

…n replacement header, and added more tests

added support for bulk update

2db63ad

added test for full repo check for incorrect licenses

8b59f83

ran dart formatter

bd73e36

fixed missing training commas

c8ec0c6

moved license utils to tool

3615813

fixed dependencies

5090415

added github action to run tool tests

d28b5b5

added write permission to tool_tests.sh

847104c

fixed Dartdoc function comment, experimenting with repo wide check te…

3e9e509

…st on CI

mossmana added 14 commits October 24, 2024 12:57

excluding flutter-sdk dependencies from repo wide check

a173b82

test that the repo wide check actually catches incorrect license header

1194a0d

Turned off repo wide check test

5541031

addressed PR feedback

5ba7ab8

addressed more PR feedback

adb5a1a

reverted some accidentally changed files, addressed additional PR fee…

95ac3b9

…dback

fixed DartDoc and other PR feedback

4ee8d9d

added dry run option for bulk update

faeb2d1

corrected logic for test

f766fa2

fixed missing trailing comma

5e15984

addressing some missed PR feedback

ac125b1

removed late and added back final to member variables

eeecf76

added clearer docs and removed unnecessary todo

af2f6bf

addressed missed PR feedback

48fc9aa

mossmana force-pushed the issue8216 branch from 29340e1 to 48fc9aa Compare October 24, 2024 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

License Utility #8460

License Utility #8460

mossmana commented Oct 21, 2024 •

edited

Loading

mossmana commented Oct 22, 2024

kenzieschmoll left a comment

kenzieschmoll Oct 22, 2024

mossmana Oct 22, 2024

bkonyi Oct 22, 2024

kenzieschmoll Oct 23, 2024

kenzieschmoll Oct 22, 2024

mossmana Oct 22, 2024

kenzieschmoll Oct 22, 2024

kenzieschmoll Oct 22, 2024

mossmana Oct 22, 2024

bkonyi Oct 22, 2024

mossmana Oct 22, 2024

mossmana Oct 22, 2024

bkonyi Oct 22, 2024

mossmana Oct 22, 2024

kenzieschmoll Oct 22, 2024

mossmana Oct 22, 2024

bkonyi Oct 22, 2024

bkonyi Oct 22, 2024

bkonyi Oct 22, 2024

bkonyi Oct 22, 2024

mossmana Oct 22, 2024

bkonyi Oct 22, 2024

bkonyi Oct 22, 2024

kenzieschmoll Oct 22, 2024

mossmana commented Oct 23, 2024

kenzieschmoll commented Oct 23, 2024

bkonyi Oct 24, 2024

bkonyi Oct 24, 2024

mossmana Oct 25, 2024

License Utility #8460

Are you sure you want to change the base?

License Utility #8460

Conversation

mossmana commented Oct 21, 2024 • edited Loading

Pre-launch Checklist

mossmana commented Oct 22, 2024

kenzieschmoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossmana commented Oct 23, 2024

kenzieschmoll commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossmana commented Oct 21, 2024 •

edited

Loading