Skip to content
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

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open

License Utility #8460

wants to merge 31 commits into from

Conversation

mossmana
Copy link

@mossmana mossmana commented Oct 21, 2024

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

#8216

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.

build.yaml badge

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

@mossmana mossmana changed the title Issue8216 License Utility Oct 21, 2024
@mossmana mossmana marked this pull request as ready for review October 22, 2024 17:25
@mossmana mossmana requested review from bkonyi and a team as code owners October 22, 2024 17:25
@mossmana
Copy link
Author

@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

Copy link
Member

@kenzieschmoll kenzieschmoll left a 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 Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
@mossmana
Copy link
Author

@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.

@kenzieschmoll
Copy link
Member

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 Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

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

Almost there!

tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
@mossmana
Copy link
Author

@bkonyi @kenzieschmoll Thanks for all the time you've invested so far! I think I managed to address all the issues, but please let me know if I didn't address it correctly or if there's more to change - hopefully not 🙏

Note: I ran into a few environment issues that made it difficult to keep my branch in sync with master. So, there were files I didn't change that were showing up in the PR. I had to cherry-pick and force push to get the branch back to a reasonable state. Hopefully, I didn't lose any of the changes that were requested and that I made.

Copy link
Contributor

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

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

This LGTM overall! Just a couple of minor comments.

tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
tool/lib/license_utils.dart Outdated Show resolved Hide resolved
Copy link
Member

@kenzieschmoll kenzieschmoll left a comment

Choose a reason for hiding this comment

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

One request for usage instructions, but LGTM overall.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants