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

Replace Placeholders in Licenses #22117

Closed
wants to merge 3 commits into from

Conversation

JakobDev
Copy link
Contributor

Some Licenses have Placeholders e.g. the BSD Licenses start with this line:

Copyright (c) <year> <owner>. 

This PR replaces the placeholders with the correct value, when creating a new Repo. Tested with the BSD, GPL, AGPL and MIT Licenses, which are the most used.

@delvh
Copy link
Member

delvh commented Dec 13, 2022

Hmm, I'm not sure if it is a good idea to manually overwrite licenses.
While this change does not seem malicious, I don't think it is a good idea to manipulate a legally binding document in any way that the original author did not intend.
If the owner of the license did not update their license to throw out the placeholder, is it really a good idea that we do it for them?
Also, doing that probably gives them the false impression that they already filled out the fields, while actually the license is still not filled out when cloning the repo…

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 13, 2022
@JakobDev
Copy link
Contributor Author

I don't think it is a good idea to manipulate a legally binding document

GitLab does already replace the placeholders in Licenses for Years now. And I think, GitLab has Lawyers who say this is not an Issue.

in any way that the original author did not intend.

I think the original Author intended this, or else he would not have used Placeholders.

If the owner of the license did not update their license to throw out the placeholder, is it really a good idea that we do it for them?

Most people do not edit the License. They just select License foo for the Project and that's all. They expect, everything is done with that and not, that they have to edit the License and remove Placeholders.

Also, doing that probably gives them the false impression that they already filled out the fields, while actually the license is still not filled out when cloning the repo…

I don't think that. Most Forks are just for making PRs. If someone makes a hard Fork, he probably takes a look at the License anyway.

@KN4CK3R
Copy link
Member

KN4CK3R commented Dec 13, 2022

Gitlab matches more placeholders:

  PROJECT_TEMPLATE_REGEX =
    %r{[\<\{\[]
      (project|description|
      one\sline\s.+\swhat\sit\sdoes\.) # matching the start and end is enough here
    [\>\}\]]}xi.freeze
  YEAR_TEMPLATE_REGEX = /[<{\[](year|yyyy)[>}\]]/i.freeze
  FULLNAME_TEMPLATE_REGEX =
    %r{[\<\{\[]
      (fullname|name\sof\s(author|copyright\sowner))
    [\>\}\]]}xi.freeze

Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

hmm... I guess licenses tend to be small but this could be significantly improved.

for example if the license does not contain a '<' there is no point doing any of the replacements. So that could be a very simple improvement.

If one were then to use a strings.Replacer, whilst this causes a copy of the bytes to be made, the original []byte could then be used as the basis of a buffer to write into.

However, I'll approve because I think the perf issues here are small enough.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 14, 2022
@lunny lunny added the type/enhancement An improvement of existing functionality label Dec 15, 2022
@lunny lunny added this to the 1.19.0 milestone Dec 15, 2022
@KN4CK3R
Copy link
Member

KN4CK3R commented Dec 16, 2022

The placeholders do not cover all the cases:

[YEAR]

Copyright [YEAR] [NAME] [EMAIL]

{YEAR}

[The following copyright notice will be used if created by a contractor pursuant to Government Agency contract and rights obtained from creator by assignment. Government Agency will insert the year and its Agency designation and remove the bracketed language.] Copyright (c) {YEAR} United States Government as represented by ___ ____. All Rights Reserved.

@KN4CK3R
Copy link
Member

KN4CK3R commented Jan 2, 2023

Why not use a regex? You are still missing placeholders.

[name of copyright owner], [yyyy] (Apache-2.0)
[name of copyright holder] (MulanPSL-2.0)
<COPYRIGHT HOLDERS> (BSD-2)
<copyright holders> (ECL-1.0)
<AUTHOR> (AAL)
<author's name or designee> (OPUBL-1.0)
[one or more legally recognised persons or entities offering the Work under the terms and conditions of this Licence] (CC-BY-NC-SA-2.0-UK)
...

@lunny lunny modified the milestones: 1.19.0, 1.20.0 Feb 4, 2023
@wolfogre
Copy link
Member

wolfogre commented Apr 26, 2023

I got inspired by this PR, and I think we could handle this in a more maintainable way: #24354.

What do you think? @JakobDev

@wxiaoguang
Copy link
Contributor

ping ~~~

Personally I think #24354 seems handling more cases and could be covered by tests

@wxiaoguang wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label May 2, 2023
@JakobDev
Copy link
Contributor Author

JakobDev commented May 2, 2023

I'm closing this PR in favour of the new one

@JakobDev JakobDev closed this May 2, 2023
@GiteaBot GiteaBot removed this from the 1.20.0 milestone May 2, 2023
@JakobDev JakobDev deleted the licenseplaceholder branch May 2, 2023 14:52
silverwind pushed a commit that referenced this pull request May 5, 2023
Replace #22117. Implement it in a more maintainable way.

Some licenses have placeholders e.g. the BSD licenses start with this
line:
```
Copyright (c) <year> <owner>. 
```
This PR replaces the placeholders with the correct value when initialize
a new repo.

### FAQ

- Why not use a regex?
It will be a pretty complicated regex which could be hard to maintain.

- There're still missing placeholders.
There are over 500 licenses, it's impossible for anyone to inspect all
of them alone. Please help to add them if you find any, and it is also
OK to leave them for the future.

---------

Co-authored-by: Giteabot <teabot@gitea.io>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail lgtm/need 1 This PR needs approval from one additional maintainer to be merged. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants