Opam: Use the 'user' option as the fork owner #480
Conversation
|
Hmm, yeah, this approach in general seems reasonable to me. I think the idea was that for 80% of users no configuration is required and it can be inferred but you're right that it is not a 100% solution so being able to explicitly provide the user sounds like a good option to me. |
|
It seems that the The problem I can see is pushing the changes to repo |
I do hope so, however depending on whether we still want to support local repos (I would say no, others might disagree) this might not be a complete solution. |
|
This has slowed me down in release Odoc today. |
|
I dug around and I didn't find anything in the API that would allow to do that without the username/ownername (with just the url as you suggested @Leonidas-from-XIV), what we are doing right now (https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#create-a-pull-request) looks like the only way. @Leonidas-from-XIV do you have more details about what you suggested, or more concerns about this PR? |
Decoding the remote URL can easily fail, instead let the user provide the correct username to use in API calls. Only attempt to decode the remote URL if the 'user' option is not set. The error message is changed to encourage setting the 'user' option instead of asking users to change their URLs.
gpetiot
force-pushed
the
fork-owner-conf
branch
from
a05f9c8
to
6c43c4d
Compare
|
I modified the code according to the review and added an entry in the changelog. |
|
LGTM, thanks! |
| @@ -164,19 +165,25 @@ let open_pr ~dry_run ~changes ~remote_repo ~fork_owner ~branch ~token ~title | |||
| | Ok () -> Ok 0 | |||
| | Error _ -> msg ()) | |||
|
|
|||
| let parse_remote_repo remote_repo = | |||
| (** Get the value from the [user] config, if it is unset try to parse it from | |||
There was a problem hiding this comment.
The docstring is now wrong.
This is an attempt at fixing #479 but I noticed that the
useroption is deprecated.Decoding the remote URL can easily fail, instead let the user provide the correct username to use in API calls.
Only attempt to decode the remote URL if the
useroption is not set. The error message is changed to encourage setting theuseroption instead of asking users to change their URLs.