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

Exclude certain URLs for local linkchecks #109

Merged
merged 3 commits into from
Oct 29, 2024
Merged

Exclude certain URLs for local linkchecks #109

merged 3 commits into from
Oct 29, 2024

Conversation

cjsha
Copy link
Member

@cjsha cjsha commented Oct 24, 2024

  • branched github URLs being checked even though they don't exist until pushed. Those URLs are now optionally checked with the -r flag
  • "linkcheck" -> "lychee" to distinguish the function that runs lychee linkchecks and the function that runs doclinkchecker linkchecks

Resolve #74

- branched github URLs being checked even though they don't exist until
  pushed. Those URLs are now optionally checked with the -r flag
- "linkcheck" -> "lychee" to distinguish the function that runs lychee
  linkchecks and the function that runs doclinkchecker linkchecks
@cjsha cjsha requested a review from bparks13 October 24, 2024 16:48
@cjsha
Copy link
Member Author

cjsha commented Oct 24, 2024

I was looking at the gh actions - afaik this doesn't affect them because the docfx-utils.ps1 script is never being run with the -a or -l flag. In any case, I don't wouldn't wanna mess those up - I'll let @bparks13 modify those if necessary.

@bparks13
Copy link
Member

I already had forgotten that we decoupled the lychee action remotely; it probably isn't worth adding a flag to the utility script, because it won't affect the actions since we are using a totally different system.

I would remove the $remote flag, and just exclude the link everytime it's run locally. The other changes are valid and we should leave the renaming as it is.

I guess we don't even need to update the README, since the only thing that's being changed is the internal call and not the alias, so that's easy.

@cjsha
Copy link
Member Author

cjsha commented Oct 24, 2024

Why did we decouple it again? Is it for the --verbose? Is it worth recoupling now that we have an option of adding a -r flag for remote builds?

@bparks13
Copy link
Member

It's decoupled because there is a Github action already packaged for lychee. If we didn't use the action, we would need to download and install the lychee executable in the Github environment for every run, which seemed like more trouble than it was worth to utilize the utility script. We can definitely revisit this issue though and see if it is worth the effort

@cjsha
Copy link
Member Author

cjsha commented Oct 25, 2024

Nah. I think that's the way to go for now. I think a "wish list" issue will be doing something similar locally so we don't have to specify the lychee location when calling docfx-utils.ps1. And maybe even also coupling local and remote lychee linkchecks.

In the meanwhile, I think the best solution is as you suggested which is to remove the $remote flag so I will do that.

@bparks13
Copy link
Member

Approved, but leaving this open in case we want to revisit and update anything before merging

@cjsha
Copy link
Member Author

cjsha commented Oct 28, 2024

I don't think I'll have any updates. I already made the "wish list" issue (#114) described in my last comment. If you have any reservations or changes you'd like to make, feel to address them. Otherwise, go ahead and merge.

@cjsha cjsha assigned cjsha and unassigned cjsha Oct 28, 2024
@bparks13 bparks13 merged commit b6aba38 into main Oct 29, 2024
3 checks passed
@bparks13 bparks13 deleted the issue-74 branch October 29, 2024 13:21
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.

Update README regarding lychee checks on new files
2 participants