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.

Sign up for GitHub

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

Jump to bottom

Missing license info #7522

Merged
merged 1 commit into from
Sep 26, 2023
Merged

Missing license info #7522

merged 1 commit into from
Sep 26, 2023

Conversation

heliocastro
Copy link
Contributor

License page has only a link but no mention of what license ORT is using

@heliocastro heliocastro requested review from a team as code owners September 18, 2023 09:02
@heliocastro heliocastro self-assigned this Sep 18, 2023
@heliocastro heliocastro force-pushed the heliocastro/missing_license_info branch from b6bcde0 to 9870147 Compare September 18, 2023 09:03
sschuberth
sschuberth reviewed Sep 18, 2023
View reviewed changes
docusaurus/docs/license.md Outdated
@@ -6,6 +6,7 @@ sidebar_position: 8

Copyright (C) 2017-2023 [The ORT Project Authors](https://github.com/oss-review-toolkit/ort/blob/main/NOTICE).

OSS Review Toolkit (ORT) is licensed under the Apache License, Version 2.0.
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't the link directly below this line enough, as the docs are also hosted in the repo the link points to?

Copy link
Contributor Author

@heliocastro heliocastro Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately no, as the moment that i show to some persons, they asked why the license is not mention there.
And is really not clear for persons to need to do a one extra jump to a not user friendly page of github as non-developers.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that the website should mention the license directly without requiring to click on a link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Ok, still it feels like duplication what we have here now. How about doing

OSS Review Toolkit (ORT) is licensed under the [Apache License, Version 2.0](https://github.com/oss-review-toolkit/ort/blob/main/LICENSE).

and omitting the "See the LICENSE" sentence from below then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, because you need something indicating for the full license text.
This is a normal practice everywhere. A good example would be the projects page in github, where the main license ( or conjunction ) is displayed on the right, with a deep link for the full 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'm not following you. The full license text is still linked. It's just that the link is now called "Apache License, Version 2.0" instead of "LICENSE".

@heliocastro heliocastro force-pushed the heliocastro/missing_license_info branch from 9870147 to d4dc182 Compare September 18, 2023 09:49
@codecov
Copy link

codecov bot commented Sep 18, 2023 •
edited
Loading

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (69db3b3) 68.03% compared to head (c39082f) 68.03%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff            @@
##               main    #7522   +/-   ##
=========================================
  Coverage     68.03%   68.03%           
  Complexity     2023     2023           
=========================================
  Files           344      344           
  Lines         16727    16727           
  Branches       2371     2371           
=========================================
  Hits          11381    11381           
  Misses         4363     4363           
  Partials        983      983
Flag Coverage Δ
funTest-non-docker 36.51% <ø> (ø)
test 35.52% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sschuberth sschuberth requested a review from a team September 19, 2023 13:52
mnonnenmacher
mnonnenmacher previously requested changes Sep 19, 2023
View reviewed changes
Copy link
Member

@mnonnenmacher mnonnenmacher left a comment

Choose a reason for hiding this comment

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

@heliocastro Please remove the merge commit and rebase your commit instead.

@heliocastro heliocastro force-pushed the heliocastro/missing_license_info branch from 86f87ab to 8377fad Compare September 19, 2023 14:56
@heliocastro
Copy link
Contributor Author

Is anything preventing this to be merged ?

@heliocastro heliocastro force-pushed the heliocastro/missing_license_info branch from 8377fad to 3afd89d Compare September 26, 2023 06:32
sschuberth
sschuberth reviewed Sep 26, 2023
View reviewed changes
website/docs/license.md Outdated Show resolved Hide resolved
@heliocastro heliocastro force-pushed the heliocastro/missing_license_info branch 5 times, most recently from d95956f to b75f485 Compare September 26, 2023 16:15
tsteenbe
tsteenbe previously approved these changes Sep 26, 2023
View reviewed changes
@tsteenbe tsteenbe dismissed mnonnenmacher’s stale review September 26, 2023 16:16

Merge commit was removed

@tsteenbe tsteenbe enabled auto-merge (rebase) September 26, 2023 16:17
sschuberth
sschuberth requested changes Sep 26, 2023
View reviewed changes
website/docs/license.md Outdated
@@ -6,8 +6,7 @@ sidebar_position: 8

Copyright (C) 2017-2023 [The ORT Project Authors](https://github.com/oss-review-toolkit/ort/blob/main/NOTICE).

See the [LICENSE](https://github.com/oss-review-toolkit/ort/blob/main/LICENSE) file in the root of this project for
license details.
Licensed under the [Apache License, Version 2.0](https://github.com/oss-review-toolkit/ort/blob/main/LICENSE)
Copy link
Member

Choose a reason for hiding this comment

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

Missing dot at end of sentence.

@heliocastro
fix(docs): Mention Apache 2.0 license
c39082f 
Signed-off-by: Helio Chissini de Castro <[email protected]>
@heliocastro heliocastro force-pushed the heliocastro/missing_license_info branch from b75f485 to c39082f Compare September 26, 2023 16:39
@tsteenbe tsteenbe merged commit f75d200 into main Sep 26, 2023
22 checks passed
@tsteenbe tsteenbe deleted the heliocastro/missing_license_info branch September 26, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sschuberth sschuberth sschuberth approved these changes

@mnonnenmacher mnonnenmacher mnonnenmacher left review comments

@tsteenbe tsteenbe tsteenbe left review comments

Assignees

@heliocastro heliocastro

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@heliocastro @sschuberth @tsteenbe @mnonnenmacher