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

Should use_github_links() really be on the release checklist? #1805

Closed
DavisVaughan opened this issue Mar 15, 2023 · 8 comments · Fixed by #1834
Closed

Should use_github_links() really be on the release checklist? #1805

DavisVaughan opened this issue Mar 15, 2023 · 8 comments · Fixed by #1834
Labels
git git, GitHub, and CI in general release 🛫

Comments

@DavisVaughan
Copy link
Member

It comes with usethis::use_release_issue()

It tries to change URL: https://vctrs.r-lib.org/ to URL: https://github.com/r-lib/vctrs, which is not what we wanted.

@hadley said this is likely a more community focused function, so we probably don't need it in our release checklist?

@jennybc
Copy link
Member

jennybc commented Mar 15, 2023

I recently had an experience when spring cleaning which made me add this commit:

dac38d0

use_github_links() is not in the release checklist unconditionally; it's based on a test for whether the URL field looks like it needs to add GitHub links (has_github_links()). So I think the preferred solution is to improve has_github_links().

What was the full before and after for vctrs? I.e. if vctrs only listed the pkgdown site, I assume you would want to add the GitHub site. So maybe there's also something here about appending?

Related: I think it's also possible that we should have use_release_issue() for the community and use_tidy_release_issue().

@jennybc jennybc added git git, GitHub, and CI in general release 🛫 labels Mar 15, 2023
@DavisVaughan
Copy link
Member Author

Yea so unlike dplyr:

URL: https://dplyr.tidyverse.org, https://github.com/tidyverse/dplyr

the vctrs URLs only include the pkgdown site

URL: https://vctrs.r-lib.org/

I'm not opposed to adding the github one too

@jennybc
Copy link
Member

jennybc commented Mar 15, 2023

Also for context, here's why it got added: #1704

@jennybc
Copy link
Member

jennybc commented Mar 15, 2023

It's definitely our de facto standard to have both the pkgdown site and the GitHub site and I really hope we're not talking about changing that.

@DavisVaughan
Copy link
Member Author

Yea it sounds like maybe the detection of the github link is working, but it should be appending to the list of existing urls rather than overwriting them entirely?

@jennybc
Copy link
Member

jennybc commented Mar 15, 2023

I think up 'til now we've basically accepted that if this function does something to your URL field ... it might not do exactly what you want. But it gets your attention, so you do something.

@ateucher
Copy link
Collaborator

I think:

  • has_github_links() is doing what it should - it looks for both the issues url in BugReports and the github repo url in URL. I think the fact that use_github_links() showed up in the vctrs release issue is correct as it only had the pkgdown site (I don't think it should detect a pkgdown site as a github link).
  • If the github repo url is not present, use_github_links() should just append it to the list of urls rather than overwrite it (as @DavisVaughan said), otherwise do nothing.
  • use_github_links() updates the BugReports url unconditionally with the issues url, which I think is fine

If this sounds right I'll do a PR so that use_github_links() appends the github url.

@jennybc
Copy link
Member

jennybc commented Apr 26, 2023

Yes @ateucher sounds like a good plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
git git, GitHub, and CI in general release 🛫
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants