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

Make all instances of doc links in logs clickable #2355

Merged
merged 4 commits into from
Jul 23, 2024
Merged

Make all instances of doc links in logs clickable #2355

merged 4 commits into from
Jul 23, 2024

Conversation

angelapwen
Copy link
Contributor

Fixes #2354.

This PR updates all log messages with links to GitHub docs so that they are not succeeded by punctuation marks, in order to make sure the links are clickable from our logs.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.
@angelapwen angelapwen mentioned this pull request Jun 27, 2024
Closed
@angelapwen angelapwen force-pushed the angelapwen/make-log-links-clickable branch from 2e6d089 to 90ece33 Compare June 27, 2024 14:27
@angelapwen angelapwen marked this pull request as ready for review June 27, 2024 14:32
@angelapwen angelapwen requested a review from a team as a code owner June 27, 2024 14:32
NlightNFotis
NlightNFotis previously approved these changes Jun 27, 2024
View reviewed changes
Copy link
Member

@NlightNFotis NlightNFotis left a comment

Choose a reason for hiding this comment

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

LGTM broadly, just a couple of nits - don't need to be implemented within the PR.
src/codeql.test.ts Outdated
@@ -912,8 +912,8 @@ test("runTool summarizes autobuilder errors", async (t) => {
instanceOf: CommandInvocationError,
message:
"We were unable to automatically build your code. Please provide manual build steps. " +
"For more information, see " +
"https://docs.github.com/en/code-security/code-scanning/troubleshooting-code-scanning/automatic-build-failed. " +
"See https://docs.github.com/en/code-security/code-scanning/troubleshooting-code-scanning/automatic-build-failed " +
Copy link
Member

@NlightNFotis NlightNFotis Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm wondering whether it makes sense to export these into their own variables, and even better perhaps in their own file, something like src/docs-urls.ts or something similar.

Doing so would have the benefit of them being more centrally organised, so we can do a one-step global replace if anything changes by just changing the value of the constant to the new URL, and have the documentation strings be automatically updated everywhere.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this in an enum, similar to the way we have EnvVar for shared environment variables 😄 it was a really good idea — from there I found other instances where we had docs links that ended with periods!
src/autobuild.ts Outdated
: ""
}`,
);
core.exportVariable(envVar, "false");
} else {
logger.info(
`Enabling ${featureName}. This can be disabled by setting the ${envVar} environment variable to 'false' (see ${envDoc}).`,
`Enabling ${featureName}. This can be disabled by setting the ${envVar} environment variable to 'false' (see ${envDoc} for more information).`,
Copy link
Member

Choose a reason for hiding this comment

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

In some of these strings we have the URL reference inside of a parenthesis, whereas in others we end the sentence with a period and start a new one.

I'm curious if there are any benefits to being more consistent on that front.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely don't see any downsides to standardizing our docs links — I removed all the parenthetical URL references!
henrymercer
henrymercer previously approved these changes Jul 1, 2024
View reviewed changes
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Nice! Non-blocking, optional: This could be something we create a CodeQL query for to help make sure we follow this pattern going forward.
@angelapwen angelapwen dismissed stale reviews from henrymercer and NlightNFotis via 10d5681 July 23, 2024 12:17
@angelapwen angelapwen force-pushed the angelapwen/make-log-links-clickable branch from 90ece33 to 10d5681 Compare July 23, 2024 12:17
@angelapwen
Further standardize doc URL messages
edfef27 
Always say "see $URL for more information" without parentheses.
@angelapwen angelapwen force-pushed the angelapwen/make-log-links-clickable branch from 10d5681 to edfef27 Compare July 23, 2024 12:18
@angelapwen
Copy link
Contributor Author

Nice! Non-blocking, optional: This could be something we create a CodeQL query for to help make sure we follow this pattern going forward.

Thanks!! I've waited too long to get back to this PR 😆 so I won't write the CodeQL query this time. I think the new DocUrl enum out should make it a lot clearer moving forward as well.
@angelapwen angelapwen force-pushed the angelapwen/make-log-links-clickable branch from 33902d8 to 892ff9e Compare July 23, 2024 13:21
@angelapwen angelapwen merged commit b400d0f into main Jul 23, 2024
310 checks passed
@angelapwen angelapwen deleted the angelapwen/make-log-links-clickable branch July 23, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants
@angelapwen @NlightNFotis @henrymercer