-
Notifications
You must be signed in to change notification settings - Fork 326
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.
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
License Utility #8460
base: master
Are you sure you want to change the base?
License Utility #8460
Conversation
@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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with first pass.
tool/lib/license_utils.dart
Outdated
YamlList removeLicenses = YamlList(); | ||
YamlList addLicenses = YamlList(); | ||
YamlList includePaths = YamlList(); | ||
YamlList excludePaths = YamlList(); | ||
YamlMap fileTypes = YamlMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make all of these final. Additionally, can we add dart doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they're final, I won't be able to overwrite them in LicenseConfig.fromYamlFile
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should probably be defined as late final Yaml{List,Map} variable
then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be final now that we are using a factory constructor.
tool/lib/license_utils.dart
Outdated
String storedName, | ||
String existingLicenseText, | ||
String replacementLicenseText, | ||
String content, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we make these required named parameters for better readability at the call site?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
existingHeader, | ||
replacementHeader, | ||
); | ||
final File backupFile = file.copySync('${file.path}.bak'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the backupFile for? it doesn't look like we do anything with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We delete backupFile
below though, don't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is an exception, the program aborts before getting to the deletion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe over architecting here, but wanted to have some fallback. Is there a better way to handle this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
tool/lib/license_utils.dart
Outdated
|
||
/// 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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It references class member functions such as getReplacementInfo
and rewriteLicenseHeader
.
tool/lib/license_utils.dart
Outdated
/// [p.extension]). | ||
String _removeDotFromExtension(String ext) { | ||
if (ext.startsWith('.')) { | ||
return ext.replaceFirst('.', ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: maybe use ext.substring(1)
instead?
tool/lib/license_utils.dart
Outdated
) async { | ||
final File rewrittenFile = File('${file.path}.tmp'); | ||
// TODO: [mossmana] Convert to using a stream | ||
await file.readAsString().then((String contents) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: avoid using .then(...)
if the code can be written sequentially using await
.
tool/lib/license_utils.dart
Outdated
replacementHeader, | ||
); | ||
final File backupFile = file.copySync('${file.path}.bak'); | ||
if (await rewrittenFile.length() > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using lengthSync()
here instead.
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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
tool/lib/license_utils.dart
Outdated
) async { | ||
final File rewrittenFile = File('${file.path}.tmp'); | ||
// TODO: [mossmana] Convert to using a stream | ||
await file.readAsString().then((String contents) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
tool/lib/license_utils.dart
Outdated
existingHeader, | ||
replacementHeader, | ||
); | ||
await rewrittenFile.writeAsString(replacementContents); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto about using the synchronous FS APIs.
tool/lib/license_utils.dart
Outdated
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@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. |
I went through and resolved open comments where possible. There are still a few open comments to address, but looking good! |
tool/lib/license_utils.dart
Outdated
required String replacementHeader, | ||
}) { | ||
final rewrittenFile = File('${file.path}.tmp'); | ||
// TODO(mossmana): Convert to using a stream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this TODO relevant anymore?
/// | ||
/// 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({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
Created a utility for updating licenses in configured source files that can be run via tests and/or standalone.
#8216
Pre-launch Checklist
///
).If you need help, consider asking for help on Discord.