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

[DISQ-8] Add license header, empty CONTRIBUTORS.md file. #13

Merged
merged 1 commit into from Oct 18, 2018

Conversation

heuermh
Copy link
Contributor

@heuermh heuermh commented Jul 19, 2018

Fixes #8.

LICENSE Outdated
@@ -1,6 +1,6 @@
MIT License

Copyright (c) 2018 nameless-gos
Copyright (c) 2018 disq-bio contributors
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it say "DISQ AUTHORS" instead? This repo has diferent authors than disq-bio organization, and with authors we pinpoint to the AUTHORS file.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is the project "Disq" (not it is not all caps).

*
* MIT License
*
* Copyright (c) 2018 disq-bio contributors
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going by the suggestions in #8.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that we said "[project name] AUTHORS", and the project is DISQ in the DISTQ-BIO organization. Otherwise, members of the whole organization holds the copyright...

@@ -0,0 +1,25 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in ADAM we have a build rule that confirms every source file has a proper license header.

Copy link
Contributor

Choose a reason for hiding this comment

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

So every file should include the license header? That it's a bit redundant, but fine to me...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's pretty standard to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that it is kind of standard, but I always doubt about adding it or not because of redundancy and easier out-of-date years when updating. If there is a rule for checking that, I am not oposed to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just specify in the root that all code is under the given license? I've never understood the need for individual license headers in every file, it's just a source of hassle and inconsistency. A build rule helps but it seems better to just say "all files in this repo are under this license".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've found lots of code "imported" into other projects without properly adhering to the license. If there is a license header on every file, it makes it explicit to the "importer" that they are breaking the license, having to remove the text of the license header.

Copy link
Member

Choose a reason for hiding this comment

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

I lean towards having a header for exactly this reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmn. I see the point there, but it still seems like more hassle for us than there is benefit.

AUTHORS Outdated
@@ -0,0 +1 @@
Contributors to disq-bio:
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend something with an explanation (sligthly modified from htsjdk-next-beta):

# This is the official list of DISQ authors for copyright purposes.
#
# The AUTHORS file lists the copyright holders.
# For example, employees from a company are not listed here, because the company holds the copyright.

# This file should be updated whenever a new contributor merges their first commit.

# Names should be added to this file as
# Organization <webpage> - Contact Person <email>
# Individual Name <email address or website>

# Please keep the list sorted alphabetically, companies/organizations first, individuals second.

Organizations:


Individuals:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hold up, I don't understand what this line means:

For example, employees from a company are not listed here, because the company holds the copyright.

Copy link
Contributor

Choose a reason for hiding this comment

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

So in htsjdk-next-beta, this line is specific to Broad Institute and it reads: "For example, Broad Institute employees are not listed here, because Broad Institute holds the copyright." I just tried to generalize, becasue no company is yet in the authors list. If you come up with a better phrasing, I'm fine with it too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really really don't like this, to the point that I'm considering giving up on this whole effort. I strongly believe the copyright has to be assigned to the project itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the concern exactly? If the code is licensed under MIT, it permits essentially anything except for relicensing under a more liberal license without any copyright assignment.

Copy link
Contributor

Choose a reason for hiding this comment

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

The copyright is hold by the project itself and its authors, which are people who contributed to the project (we should decide if with code only or other types of contribution can apply).

A recognizition model in the AUTHORS file is a good way to encourage contributions.

Copy link
Member

Choose a reason for hiding this comment

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

@heuermh I'm sorry you feel this way - I thought we were proceeding well so far since we had come to some consensus over copyright (e.g. #8 (comment)). Anyway, I hope you can reconsider.

@heuermh
Copy link
Contributor Author

heuermh commented Sep 24, 2018

Hello @magicDGS, I believe I've addressed review comments in the last commit. Could you revisit your review?

Copy link
Contributor

@magicDGS magicDGS left a comment

Choose a reason for hiding this comment

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

One comment not blocking the merge and a final request before merging: could you maybe rename the AUTHORS file to CONTRIBUTORS? Otherwise, the license and the file should say "Contributors to Disq" and "Disq contributors", respectively, to be consistent with the phrasing.

After that, LGTM 👍 (feel free to merge by yourself)

AUTHORS Outdated
@@ -0,0 +1 @@
Contributors to Disq:
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add to the contributing guidelines how the name "Disq" should be written in files mentioning it: so disallow "DISQ", "distq" or depending on the file using it (java class, etc.).

@heuermh
Copy link
Contributor Author

heuermh commented Sep 25, 2018

One comment not blocking the merge and a final request before merging: could you maybe rename the AUTHORS file to CONTRIBUTORS?

Sure, that sounds fine. I'll do that in a commit and force push to squash.

@heuermh heuermh changed the title [DISQ-8] Add license header, empty AUTHORS file. [DISQ-8] Add license header, empty CONTRIBUTORS.md file. Sep 25, 2018
@heuermh heuermh mentioned this pull request Oct 17, 2018
Copy link
Member

@tomwhite tomwhite left a comment

Choose a reason for hiding this comment

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

I think this can be merged now.

@tomwhite tomwhite merged commit 8ff1c6e into disq-bio:master Oct 18, 2018
@tomwhite
Copy link
Member

Thanks @heuermh and @magicDGS!

@heuermh
Copy link
Contributor Author

heuermh commented Oct 18, 2018

Thank you, @tomwhite

@heuermh heuermh deleted the copyright branch October 18, 2018 14:52
@magicDGS
Copy link
Contributor

Thank you!

@heuermh heuermh added this to the 0.1.0 milestone Oct 24, 2018
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.

None yet

4 participants