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

docs(README): Fix several broken links #7700

Merged
merged 3 commits into from
Nov 13, 2023
Merged

docs(README): Fix several broken links #7700

merged 3 commits into from
Nov 13, 2023

Conversation

fviernau
Copy link
Member

No description provided.

@fviernau fviernau requested a review from a team as a code owner October 15, 2023 09:22
@fviernau fviernau mentioned this pull request Oct 15, 2023
Merged
@codecov
Copy link

codecov bot commented Oct 15, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (17f3ad1) 67.06% compared to head (7cb0ae1) 67.06%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff            @@
##               main    #7700   +/-   ##
=========================================
  Coverage     67.06%   67.06%           
  Complexity     2042     2042           
=========================================
  Files           356      356           
  Lines         17045    17045           
  Branches       2438     2438           
=========================================
  Hits          11432    11432           
  Misses         4593     4593           
  Partials       1020     1020
Flag Coverage Δ
funTest-non-docker 34.95% <ø> (ø)
test 36.10% <ø> (ø)

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.

@fviernau fviernau force-pushed the readme-link-fixes branch 2 times, most recently from f4970b3 to e56ec04 Compare October 15, 2023 10:02
@fviernau fviernau changed the title docs(README): Fix links to configuration file docs docs(README): Fix several broken links Oct 15, 2023
@fviernau fviernau enabled auto-merge (rebase) October 15, 2023 10:16
sschuberth
sschuberth requested changes Oct 15, 2023
View reviewed changes
README.md Show resolved Hide resolved
@fviernau fviernau requested a review from sschuberth October 16, 2023 11:08
@sschuberth
Copy link
Member

Please also rebase to skip the first commit, which should be equal to @nnobelis's change.

@fviernau fviernau disabled auto-merge October 17, 2023 06:53
sschuberth
sschuberth reviewed Oct 17, 2023
View reviewed changes
README.md
@@ -116,7 +116,7 @@ Depending on how ORT was installed, it can be run in the following ways:
docker run ort --help
```

You can find further hints for using ORT with Docker in the [documentation](./docs/hints-for-use-with-docker.md).
You can find further hints for using ORT with Docker in the [documentation](./website/docs/guides/docker.md).
Copy link
Member

Choose a reason for hiding this comment

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

Note that these changes might become obsolete with #7518, but as we don't know how long it will take to merge, I guess it's better to fix these now, even if temporarily.

README.md Outdated
gathered in preceding stages and returns a list of policy violations, e.g. to flag license findings.
* [*Reporter*](#reporter) - presents results in various formats such as visual reports, Open Source notices or
Bill-Of-Materials (BOMs) to easily identify dependencies, licenses, copyrights or policy rule violations.
* [*Analyzer*](./website/docs/tools/analyzer.md) - determines the dependencies of projects and their metadata,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, the commit message is a bit misleading, as they links were not actually broken (when browsing the README.md on the GitHub project page). I'm unsure whether it is a better user experience to jump from the GitHub README.md to the documentation side than staying at GitHub and jumping to the respective tool section. What do you think @mnonnenmacher?

Copy link
Member Author

@fviernau fviernau Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ping: @mnonnenmacher Could you reply to @sschuberth 's ask above to unblock this, please?

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 as long as the sections exist in the README it's indeed a better UX to not leave the peage. I'd rather do the cleanup of the README, this will probably remove large parts of the README and then changing the links makes more sense. I'll try to prepare a draft for the next core dev meeting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shall I close this PR then ?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should close this completely, as there are some valid changes in here that would fix the link check errors I'm running into e.g. here.

So basically, I believe we should drop the changes in this hunk, but keep the 3 link fixes below.

@fviernau fviernau requested a review from sschuberth November 10, 2023 10:31
@fviernau fviernau enabled auto-merge (rebase) November 10, 2023 10:31
sschuberth
sschuberth previously approved these changes Nov 10, 2023
View reviewed changes
@fviernau
docs(README): Fix further broken links
1394f0d 
Signed-off-by: Frank Viernau <[email protected]>
@fviernau
docs(README): Remove a broken link for the Notifier bullet point
0565c3d 
The notifier does not have it's own section / page under the `tools`
documentation anymore. So, just mention the notifier without linking to
align with `intro.md`.

Signed-off-by: Frank Viernau <[email protected]>
@fviernau
docs(README): Fix the link to version control system implementations
7cb0ae1 
This is a fix-up for [1].

[1]: #7836

Signed-off-by: Frank Viernau <[email protected]>
@fviernau fviernau requested a review from sschuberth November 10, 2023 12:41
sschuberth
sschuberth approved these changes Nov 10, 2023
View reviewed changes
README.md
@@ -429,7 +429,7 @@ or [packages](./plugins/package-managers/spdx/src/funTest/assets/projects/synthe

Taking an ORT result file with an *analyzer* result as the input (`-i`), the *downloader* retrieves the source code of
all contained packages to the specified output directory (`-o`). The *downloader* takes care of things like normalizing
URLs and using the [appropriate VCS tool](./downloader/src/main/kotlin/vcs) to check out source code from version
URLs and using the [appropriate VCS tool](./plugins/version-control-systems) to check out source code from version
Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@fviernau fviernau disabled auto-merge November 13, 2023 07:34
@fviernau fviernau merged commit 551c79f into main Nov 13, 2023
@fviernau fviernau deleted the readme-link-fixes branch November 13, 2023 07:34
@fviernau
Copy link
Member Author

Failing tests we're all unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnonnenmacher mnonnenmacher mnonnenmacher left review comments

@sschuberth sschuberth sschuberth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fviernau @sschuberth @mnonnenmacher