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 github_remote_list() instead of github_remotes() when GitHub not consulted #2055

Closed
wants to merge 2 commits into from

Conversation

hadley
Copy link
Member

@hadley hadley commented Sep 10, 2024

To reduce some of the usage of github_remotes() where it's not strictly necessary.

To reduce some of the usage of `github_remotes()` where it's not strictly necessary.
@hadley hadley requested a review from jennybc September 10, 2024 13:44
@hadley hadley changed the title Use github_remote_list() for testing Use github_remote_list() instead of github_remotes() when github not used Sep 10, 2024
@hadley hadley changed the title Use github_remote_list() instead of github_remotes() when github not used Use github_remote_list() instead of github_remotes() when GitHub not consulted Sep 10, 2024
@jennybc
Copy link
Member

jennybc commented Sep 10, 2024

My main reaction is: surely I had some reason for using github_remotes(github_get = FALSE) instead of github_remote_list() (?). Because obviously I had both fully uploaded into my brain when I did this. I'm just getting some Chesterton's Fence vibes. It's hard to know what will happen since none of this is under test.

@hadley
Copy link
Member Author

hadley commented Sep 10, 2024

new_*() are only used for tests, so I think that's safe.

@jennybc
Copy link
Member

jennybc commented Sep 10, 2024

new_*() are only used for tests, so I think that's safe.

I do have some memory that the whole reason I made the new_*() functions this way was so that I could have authentically "shaped" github remote info.

I.e. sure it is github remote info that is fake, but I make it in such a way that it's guaranteed to have the same "shape" as when we actually do call github. I think this can't be detected today (meaning: tests aren't failing), because the assignments we go on to make happen to create all the right fields. The fields that are no longer there due to the switch from github_remotes(github_get = FALSE) to github_remote_list() all get created now, instead of just modified.

@jennybc
Copy link
Member

jennybc commented Sep 10, 2024

So I think this implies that the PR should probably only propose a change in pr_create()?

@hadley
Copy link
Member Author

hadley commented Sep 20, 2024

Giving up on this approach

@hadley hadley closed this Sep 20, 2024
@hadley hadley deleted the use-github-remote-list branch September 20, 2024 21:50
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