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

Remove excessive copyright/license boilerplate #311

Closed
wants to merge 1 commit into from
Closed

Remove excessive copyright/license boilerplate #311

wants to merge 1 commit into from

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Jan 12, 2015

The copyright and license notice is already in the LICENSE file. There
is no justifiable reason to also require that it be included in every
file, since the individual files are not individually distributed except
as part of the entire package.

Since 1,105 files are changed in this diff, reviewers may need to pull it and check manually. I went through the list myself, and only lib/_tls_wrap.js had any unexpected issues, because of a stray comment in the middle of the copyright block that appeared to be the result of a past merge conflict oversight. (As if we needed more evidence that these copyright blocks are a troublesome eyesore!)

@isaacs isaacs mentioned this pull request Jan 12, 2015
@cole-gillespie
Copy link

+1

@@ -1,24 +1,3 @@
diff --git a/lib/_stream_duplex.js b/lib/_stream_duplex.js
Copy link
Member

Choose a reason for hiding this comment

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

Wut? Is it relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, indeed, that is weird. I'll back that out.

@indutny
Copy link
Member

indutny commented Jan 12, 2015

I guess, the sed script gone wilde? ;) Other than minor issue - LGTM

@bnoordhuis
Copy link
Member

The changes to deps/npm will be undone the next time it's upgraded, I think?

@isaacs
Copy link
Contributor Author

isaacs commented Jan 12, 2015

Backed out changes to deps. That was a mistake. We'll have to do a similar de-boilerplate change in other places, maybe.

@isaacs
Copy link
Contributor Author

isaacs commented Jan 12, 2015

Script:

#!/usr/bin/env bash

files=($(git grep -l '// Copyright Joyent, Inc. and other Node contributors.'))

for f in "${files[@]}"; do
  echo $f
  tail -n +21 "$f" > "$f".bak
  mv "$f".bak "$f"
  while [ "$(head -n1 "$f")" = "" ] && [ "$(wc -l "$f" | awk '{print $1}')" -gt 0 ]; do
    tail -n +2 "$f" > "$f".bak
    mv "$f".bak "$f"
  done
done

@piscisaureus
Copy link
Contributor

grep "copyright" -i -r * | grep -i "ben noordhuis"

@bnoordhuis
Copy link
Member

I think this PR already needs a rebase. Things move fast around here.

The copyright and license notice is already in the LICENSE file.  There
is no justifiable reason to also require that it be included in every
file, since the individual files are not individually distributed except
as part of the entire package.
@isaacs
Copy link
Contributor Author

isaacs commented Jan 12, 2015

Rebased on v1.x

On Mon, Jan 12, 2015 at 2:05 PM, Ben Noordhuis notifications@github.com
wrote:

I think this PR already needs a rebase. Things move fast around here.


Reply to this email directly or view it on GitHub
#311 (comment).

@isaacs
Copy link
Contributor Author

isaacs commented Jan 12, 2015

@piscisaureus The stuff in deps, and anything not covered by the LICENSE file, should still have its obnoxious boilerplate. If Ben wants to remove his copyright notices, I'll leave that to @bnoordhuis.

@bnoordhuis
Copy link
Member

The only things that are (c) me are src/queue.h and a test from my punycode library. The former is a verbatim copy from libuv, the latter can be stripped for all I care.

Apropos this PR, rubber-stamp LGTM.

@bnoordhuis
Copy link
Member

@isaacs I see you landed this but you forgot the PR-URL and Reviewed-By tags.

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

Successfully merging this pull request may close these issues.

5 participants