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

Update some docs links #2571

Closed
wants to merge 65 commits into from

Conversation

SeanKilleen
Copy link
Contributor

@SeanKilleen SeanKilleen commented Sep 15, 2022

Supports #2474.

This PR:

  • Converts the links in markdown
  • Removes the Beta packages section of the README, which had bad links
  • Moves all current docs.github.com links to be https.
  • Removes /en from existing routes.

Remaining Work

  • Convert Markdown links
  • Decide what to do about the README beta packages section
  • Remove /en from all changed links (docs know how to handle language now)
  • Correct http links in changed links

@SeanKilleen SeanKilleen changed the title WIP: Update docs links WIP: Update docs links and add link checker Sep 15, 2022
@SeanKilleen
Copy link
Contributor Author

@nickfloyd as part of adding the GH Action to check links, I saw that the following text in README appears to 404:

### Beta packages ###

Unstable NuGet packages that track the `main` branch of this repository are available at
[https://ci.appveyor.com/nuget/octokit-net](https://ci.appveyor.com/nuget/octokit-net)

Is there a new beta packages link? Or should I remove that section?

⚠️ I'm not sure if this is the right path. Should we update the gist list links to point to the most specific docs possible?
Find: `https://(developer.github.com)/v3/(activity|issues|orgs|pulls|repos|gists|users|git|apps|search|reactions|checks)/\n`
Replace: `https://docs.github.com/rest/$2/\n`

Reviewed all results by hand.
Find: `https://(developer.github.com)/v3/(activity|issues|orgs|pulls|repos|gists|users|git|apps|search|reactions|checks)"`
Replace: `https://docs.github.com/rest/$2/"`

Reviewed by hand.
Find: `https://(developer.github.com)/v3/(activity|issues|orgs|pulls|repos|gists|users|git|apps|search|reactions|checks)/"`
Replace: `https://docs.github.com/rest/$2/"`
@SeanKilleen
Copy link
Contributor Author

SeanKilleen commented Sep 15, 2022

Hmm....tried configuring Lychee to fail when links are redirected, since most of the links we're seeing are 301 permanent redirects. However, setting args: --max-redirects 0 in the github action YAML didn't do what I expected it to. Will fix it another time.

@nickfloyd
Copy link
Contributor

nickfloyd commented Sep 15, 2022

@nickfloyd as part of adding the GH Action to check links, I saw that the following text in README appears to 404:

### Beta packages ###

Unstable NuGet packages that track the `main` branch of this repository are available at
[https://ci.appveyor.com/nuget/octokit-net](https://ci.appveyor.com/nuget/octokit-net)

Is there a new beta packages link? Or should I remove that section?

That should now be removed - good catch, thx! ❤️. Also, you are an absolute hero for picking this one up! Thanks for giving it a go!

@SeanKilleen
Copy link
Contributor Author

SeanKilleen commented Sep 15, 2022

ℹ️ 💡 @nickfloyd while changing links in the MiscellaneousClient, I noticed that the misc docs now redirect to https://docs.github.com/en/rest/emojis

The MiscellaneousClient should likely be split up into MarkownClient, EmojiClient, GitignoreClient, etc. and then have the larger MiscellaneousClient retired in a future major release. Noting it here because you may want to add it to a backlog somewhere (happy to create an issue if you'd like).

For now, I will remove the top-level "misc" link as it no longer exists, and will add text to refer to the doc link at the method level.

@nickfloyd
Copy link
Contributor

Noting it here because you may want to add it to a backlog somewhere (happy to create an issue if you'd like).

If you get the chance, please feel free to create an issue so that we can get what you're suggesting here implemented. Thanks!

@SeanKilleen
Copy link
Contributor Author

ℹ️ Realizing that lychee doesn't support anchor tags so its usefulness will be limited in this case. Will likely end up removing it, as the majority of the links are in C# files and we'd need to convert them to UTF-8 first and then I'm still not sure we'd get the benefit.

@SeanKilleen
Copy link
Contributor Author

@nickfloyd question for you -- how important is linking to the correct anchor tag on the page?

If we wanted to go more toward the 80/20 rule, we could remove the anchor references in the links and link to the correct page. This would let us update the links quickly, but we'd lose some specificity, and the links would be more fragile as heading/anchor names change.

I'm fine to proceed either way, but if you don't care about anchor-level specificity, I can remove them.

It turned out not to provide a ton of value:

* Would have taken some adjustment for C# files to be converted to UTF-8 to be scanned with it
* Doesn't have the ability to check anchor links, where there's a lot of drift here.
@SeanKilleen
Copy link
Contributor Author

ℹ️ Removed lychee -- it wasn't going to provide enough value to guide the process and I'm not sure a lot of the drift would have been detected by it going forward. Plus, it would take a bit of elbow grease to get it to work with the C# files.

@SeanKilleen SeanKilleen changed the title WIP: Update docs links and add link checker Update docs links and add link checker Sep 15, 2022
@SeanKilleen SeanKilleen changed the title Update docs links and add link checker Update some docs links Sep 15, 2022
@SeanKilleen
Copy link
Contributor Author

SeanKilleen commented Sep 15, 2022

@nickfloyd I realized that this is going to be a larger effort than thought so I'll split it over multiple PRs.

I'll probably update #2474 with a breakdown list so that we can slice it in more meaningful ways going forward.

I'm going to do a review pass for basic fixes & comments and then it'll be ready for review / merge and I'll zoom out prior to next steps.

@SeanKilleen
Copy link
Contributor Author

Actually as much as this pains me, I'm going to close this PR and start over because:

  • I don't like the way it's structured overall -- changes aren't targeted, too hard to review.
  • I didn't realize and I accidentally changed all the whitespace in C# files due to VS Code formatting on save. Again, very hard to review.

Look for smaller, targeted PRs toward this task going forward.

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.

2 participants