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

Include license in package #3621

Merged
merged 4 commits into from May 3, 2023
Merged

Include license in package #3621

merged 4 commits into from May 3, 2023

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented May 2, 2023

Hopefully then the license should appear in Visual Studio, similar to MS packages:

image

Right now it looks like this:

image

@Shane32 Shane32 requested a review from sungam3r May 2, 2023 18:27
@Shane32 Shane32 self-assigned this May 2, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #3621 (7a37029) into master (1ae26bf) will increase coverage by 0.01%.
The diff coverage is 95.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #3621      +/-   ##
==========================================
+ Coverage   83.91%   83.93%   +0.01%     
==========================================
  Files         381      381              
  Lines       16876    16888      +12     
  Branches     2715     2717       +2     
==========================================
+ Hits        14162    14175      +13     
  Misses       2068     2068              
+ Partials      646      645       -1     
Impacted Files Coverage Δ
src/GraphQL/BoolBox.cs 53.84% <ø> (ø)
src/GraphQL/Extensions/AuthorizationExtensions.cs 59.63% <ø> (ø)
src/GraphQL/Extensions/GraphQLExtensions.cs 76.55% <ø> (ø)
src/GraphQL/Introspection/ISchemaComparer.cs 75.00% <ø> (ø)
src/GraphQL/Resolvers/NameFieldResolver.cs 96.77% <ø> (ø)
...s/Composite/AutoRegisteringInputObjectGraphType.cs 100.00% <ø> (ø)
...pes/Composite/AutoRegisteringInterfaceGraphType.cs 97.36% <ø> (ø)
.../Types/Composite/AutoRegisteringObjectGraphType.cs 100.00% <ø> (ø)
.../Types/Scalars/Enumeration/EnumerationGraphType.cs 94.54% <ø> (ø)
src/GraphQL/Utilities/SchemaPrinter.cs 85.26% <ø> (ø)
... and 2 more

@sungam3r
Copy link
Member

sungam3r commented May 2, 2023

A long time ago we switched from PackageLicenseFile to PackageLicenseExpression. Also it was a recommended way by MS, compiler reported a warning. Many projects did so. So why to bring PackageLicenseFile back again? As I understand VS (with some plugin installed?) shows package contents. OK. It is just an auxilary functionality of IDE. Perhaps later VS will learn to show license using PackageLicenseExpression. But now it cannot and it should not follow from this that the license file must be added to the package.

@Shane32
Copy link
Member Author

Shane32 commented May 2, 2023

I'm fine removing PackageLicenseFile. Nevertheless, I think the license.md file should be bundled within the nuget.

src/Directory.Build.targets Outdated Show resolved Hide resolved
Copy link
Member

@sungam3r sungam3r left a comment

Choose a reason for hiding this comment

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

Well, generally I'm not against it as well but have you seen any such usages besides MS packages? Existence of those 3 files may be explained by MS corporate politics and does not apply to us as well as majority of other open source projects.

@Shane32
Copy link
Member Author

Shane32 commented May 3, 2023

Well the license agreement explicitly says:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

“Should we not follow our own rules?” was my thinking

@Shane32
Copy link
Member Author

Shane32 commented May 3, 2023

Note that the license “expression” has no legal bearing on the copyright status (in my mind), as we (the authors) specified an explicit license / copyright notice for this package.

@Shane32 Shane32 merged commit f72c05f into master May 3, 2023
8 checks passed
@Shane32 Shane32 deleted the include_license branch May 3, 2023 05:51
@sungam3r
Copy link
Member

sungam3r commented May 3, 2023

Legal stuff is not in my strong skills.

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

3 participants