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

use_pkgdown_github_pages() causes warning in urlchecker #1526

Closed
malcolmbarrett opened this issue Oct 20, 2021 · 6 comments · Fixed by #1527
Closed

use_pkgdown_github_pages() causes warning in urlchecker #1526

malcolmbarrett opened this issue Oct 20, 2021 · 6 comments · Fixed by #1527

Comments

@malcolmbarrett
Copy link
Collaborator

malcolmbarrett commented Oct 20, 2021

And on CRAN checks.

I believe it's because the trailing / is removed, as the corrected URL has the trailing /:

site_url <- sub("/$", "", site$html_url)

However, this is a peculiar thing to find in the code, which makes me think it fixes some other problem.

Here's the result from tidysmd

urlchecker::url_check()
#> fetching [ 0 / 8 ]fetching [ 1 / 8 ]fetching [ 2 / 8 ]fetching [ 3 / 8 ]fetching [ 4 / 8 ]fetching [ 5 / 8 ]fetching [ 6 / 8 ]fetching [ 7 / 8 ]                       
#> x Error: README.md:12:56 404: Not Found
#> status](https://www.r-pkg.org/badges/version/tidysmd)](https://CRAN.R-project.org/package=tidysmd)
#>                                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#> ! Warning: DESCRIPTION:12:5 Moved
#>     https://malcolmbarrett.github.io/tidysmd
#>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#>     https://malcolmbarrett.github.io/tidysmd/

Created on 2021-10-20 by the reprex package (v2.0.1)

@jennybc
Copy link
Member

jennybc commented Oct 20, 2021

Maybe whether we want or can have the trailing / varies depending on the form of the URL. E.g., https://usethis.r-lib.org is OK, but https://malcolmbarrett.github.io/tidysmd/ is not?

Also I cannot help but note the ridiculousness of this URL grooming situation we find ourselves in 🙄

@malcolmbarrett
Copy link
Collaborator Author

😅

Maybe this is as simple as moving sub() to tidyverse_url(), then... at least for these two particular types of URLs

@jennybc
Copy link
Member

jennybc commented Oct 21, 2021

https://searchfacts.com/url-trailing-slash/

The short answer is that the trailing slash does not matter for your root domain or subdomain. Google sees the two as equivalent.

But trailing slashes do matter for everything else because Google sees the two versions (one with a trailing slash and one without) as being different URLs.

Yes, short of really getting into the URL parsing business, I think your suggestion re: moving the sub() is probably the best move.

@malcolmbarrett
Copy link
Collaborator Author

Since this is limited to GH pages URLs, the simple solution seems best. But another option could be to call urlchecker::url_update() on the fly (or maybe silently run urlchecker::url_check() and suggest the user fix via ui_todo())

@jennybc
Copy link
Member

jennybc commented Oct 21, 2021

I have a strong preference for the simple solution.

@malcolmbarrett
Copy link
Collaborator Author

I think that's reasonable. In theory, the release checklist should have caught this or other URL issues, anyway; I just happened to run use_pkgdown_github_pages() after urlchecker

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 a pull request may close this issue.

2 participants