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

misc: rename license header to use Lighthouse Authors #10469

Merged
merged 5 commits into from
Mar 18, 2020

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Mar 16, 2020

Summary
Ready to go pending approval in upcoming Lighthouse Authors meeting :)

https://opensource.google/docs/releasing/contributions/#copyright states that projects with high volume of external contributions may use "The insert project name Authors" instead of "Google Inc." in the copyright header and supply an AUTHORS file at the root of the repo where contributors may optionally add their name if they wish to be recognized. We already have such a file and contributors are free to open PRs to add themselves if they so desire.

Created with

find . -type f \( -name "*.js" -o -name "*.css" -o -name "*.html" -o -name "*.d.ts" -o -name "*.hbs" -o -name "*.rb" -o -name "*.sh" -o -name "*.json" \) \! -path "./node_modules/*" \! -path "./.git/*" | xargs gsed -i'' 's/Google Inc. All Rights Reserved./The Lighthouse Authors. All Rights Reserved./g'
git checkout master lighthouse-core/test/results/artifacts/artifacts.json

Related Issues/PRs
#10460 (comment)

@patrickhulce patrickhulce requested a review from a team as a code owner March 16, 2020 20:34
@patrickhulce patrickhulce requested review from connorjclark and removed request for a team March 16, 2020 20:34
Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

i personally reviewed every file

@connorjclark
Copy link
Collaborator

heh, smoke test failed (bytes)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM! Should probably also

  • update AUTHORS to match new text in https://opensource.google/docs/releasing/authors/
    (Google LLC is probably important, the explanatory text seems nicer, and we shouldn't prompt folks to add an email if they don't want to (even if their email is still in the git log))
  • add a note in CONTRIBUTING.md about adding their name to AUTHORS if they want

@@ -239,6 +239,6 @@ variants/core-js-3-preset-env-esmodules
variants/only-plugin
2770 total
1152 variants/only-plugin/-babel-plugin-transform-spread/main.bundle.min.js
827 variants/only-plugin/-babel-plugin-transform-regenerator/main.bundle.min.js
Copy link
Member

Choose a reason for hiding this comment

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

based on this last commit adding it, I assume this is expected? (+23 bytes seems like a weird delta?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no this was completely unexpected, but given that this test is only run when certain files get touched (and this PR touched all of them), i'm assuming it shifted ever so slightly due to a yarn update at some point.

if we care about being more rigorous here we can try and trace down the bisect and add those files to our set of "changed files" to test on, but personally I don't think it's critical enough.

@connorjclark do you disagree?

Copy link
Member

Choose a reason for hiding this comment

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

ah, got it, I assumed this was also something about changing byte length due to the license change, but that's not supported by the file name I guess :)

I'm not so worried about rigor for this PR, but stability is something we should think about for future PRs from folks who didn't write or review the generator for this test file so won't know what to make of a change in this file :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we need a lockfile for scripts/legacy-javascript/package.json

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

Successfully merging this pull request may close these issues.

4 participants