Add setup-fork command, reimplement clone command as its child
#293
Conversation
llucax
added
the
type-breaking
There was a problem hiding this comment.
Thanks for taking the time to tackle so many big issues!
I'm very keen to merge this PR, but I have a few comments and change requests. Please look at the individual comments.
Also, if you could write some release notes for the change, it will make making the next release much easier for me. You should create a file like relnotes/setup-fork.feature.md with the following format:
### Short summary
Longer description of the new features (new command, etc.).And maybe since this should deprecate postclone, also include some migration notes in relnotes/setup-fork.migration.md with the same format (short summary + a longer explanation directed to the users about what to do, in this case it should be enough to tell them to rename the postclone hook to post-setup-fork if they are using it).
If you want more details on how to write release notes, please have a look at https://github.com/sociomantic-tsunami/neptune/blob/v0.x.x/doc/library-contributor.rst#release-notes
Thanks again!
| cmd_required_config = ['username', 'oauthtoken'] | ||
| cmd_help = 'clone a GitHub repository (and fork as needed)' | ||
| cmd_usage = '%(prog)s [OPTIONS] [GIT CLONE OPTIONS] REPO [DEST]' | ||
| cmd_help = 'fork a GitHub repository' |
There was a problem hiding this comment.
This is still a maybe right? If I understood correctly, if the fork already exists it will just set up the local repo. If so, it might be better to make this more explicit here.
|
|
||
| @classmethod | ||
| def setup_parser(cls, parser): | ||
| parser.add_argument('repository', metavar='REPO', | ||
| parser.add_argument('repository', metavar='REPO', nargs='?', |
There was a problem hiding this comment.
The help here should probably mention what happens if this argument is omitted.
| os.chdir(dest) | ||
| fetchremote = args.forkremote if triangular else args.upstreamremote | ||
| remote_url = repo['parent'][urltype] | ||
| if dest is not None: |
There was a problem hiding this comment.
This code will only ever used when invoking this function from the clone command, right? If this is correct, wouldn't it make more sense to just move this code to the clone command?
If not, wouldn't it make sense to rename dest to clone_to or something like that to make it more explicit that a clone can happen here?
In general I'm not a fan of such a large method, so If you find any way to factor out some parts, it would be greatly appreciated. Maybe just remove some small parts as independent methods and then write slightly longer run methods for setup-fork and clone would make more sense to me.
| 'Fetching from {} ({})'.format(fetchremote, remote_url)) | ||
| run_hookscript('postclone', env=dict( | ||
| git_config('triangular', value='true' if triangular else 'false') | ||
| run_hookscript('post-setup-fork', env=dict( |
There was a problem hiding this comment.
This makes a lot of sense, but is a bit unfortunate that this PR comes right after v2.1.0 was released, because it now constitute a breaking change :( (and we are quite strict about following semver).
Making it a non-breaking change should be easy enough though. We can just deprecate postclone and accept both until the next major is released and we can get rid of the old name. So if you can accept postclone as a hook name too and emit a warning saying the name is deprecated and will be removed in the future, everything should be fine to put this change in v2.2.0.
You can remove the old name from the documentation, don't worry about that.
| @@ -581,6 +604,88 @@ from. These are the git config keys used: | |||
| [1] https://developer.github.com/v3/pulls/#get-a-single-pull-request | |||
|
|
|||
|
|
|||
| EXAMPLES | |||
| ======== | |||
There was a problem hiding this comment.
Nice! I'm not sure if you saw we have a bunch of examples in the README too. Not sure what to do with this, because it is definitely nice to have example both in the "home page" and in the man, and AFAIK there is no easy way to include them in both without duplicating them. So feel free to chose what makes more sense to you, but it might be good to have some sort of consistency between both set of examples. Not sure if you looked at the examples in the README.
|
Oh, the CI is also failing. There seems to be a problem generating the man page, there is probably some issue with the |
| git_remotes = git('remote', 'show', '-n').split('\n') | ||
| remote = args.upstreamremote | ||
| if remote not in git_remotes: | ||
| die("No REPO specified, nor does `{}` remote exist", remote) |
There was a problem hiding this comment.
| die("No REPO specified, nor does `{}` remote exist", remote) | |
| parser.error("No REPO specified, nor does `{}` remote exist".format(remote)) |
This will use the parse to report the usage too.
| personal = False | ||
| if upstream is None: | ||
| if args.upstreamremote != args.forkremote: | ||
| die('You are setting up with a personal repository as upstream, ' |
There was a problem hiding this comment.
| die('You are setting up with a personal repository as upstream, ' | |
| parser.error('You are setting up with a personal repository as upstream, ' |
|
I'm trying this out with the case of using no fork and I'm having issues creating a PR. It seems the |
This is actually a different use case. I tried an only-upstream configuration, and it is actually possible to achieve by setting the If you want to set up an *upstream-only* repo (no fork is present, centralized approach)::
git clone https://github.com/sociomantic-tsunami/git-hub
cd git-hub
git-hub setup-fork -U origin -F origin -f sociomantic/git-hub
This will create new branches in the upstream repo directly and no fork will be used. ``--triangular`` will be force to ``false``.Where |
|
ping @oxij ? 😢 |
|
Those are good suggestions, except I'm not sure how do I go about implementing the "hook"-related one, the hook script is a single string there.
Anyway, I was thinking, it probably makes more sense to just collect all of those and other related commands into a subcommand, like `git hub repo`.
E.g: `git hub repo fork <remote-url>`, `git hub repo setup <lots of options>`, `git hub repo clone <remote-url>`, etc.
|
You just accept both
I'm neutral about this, it would be probably nicer if managing how the git repo is related to the GitHub repo gets more complicated but again it would be a breaking change, so at least Also, please let me know if I can help in any way, if you don't mind me pushing commits to your branch I can do some of the cleanups/changes I suggested. |
|
> Those are good suggestions, except I'm not sure how do I go about implementing the "hook"-related one, the hook script is a single string there.
You just accept both `postclone` and `post-setup-fork` and if `postclone` was used, print a warning about it being deprecated.
Yes, but the hook is a script. Do you want to run it with both arguments? How do you check it did anything at all to deprecate the old one?
git's own hooks are files, there it's easy to check the file exists, the hook thing here is a shell script, you could do some primitive code analysis on it, but, well...
> Anyway, I was thinking, it probably makes more sense to just collect all of those and other related commands into a subcommand, like `git hub repo`.
I'm neutral about this, it would be probably nicer if managing how the git repo is related to the GitHub repo gets more complicated but again it would be a breaking change, so at least `git hub clone` should be still supported (although deprecated). Because of this I don't mind keeping it dirty for now (and do a cleanup on a future major release). I prefer merging these changes sooner than later, even if not perfect.
Alright.
Also, please let me know if I can help in any way, if you don't mind me pushing commits to your branch I can do some of the cleanups/changes I suggested.
Sure, feel free to do whatever with the code here. I'm a bit busy with other stuff ATM.
|
This command greatly simplifies setup for a previously clonned repository. A somewhat unforturate consequence is that the name of the only `hub.hookscript` had to be changed since invoking that hook is part `setup-fork` now. Since I had to rename it anyway I also renamed it to match git's own hook naming convention of using "-"es to separate words.
Right, right, right. I completely forgot it was one script and the type of hook was passed as argument (my impression was this feature was too niche to deserve having many hook scripts). Then yes, calling the script twice would be an option (although not very nice). To be honest the cost/benefit of the rename at this point doesn't seem to be worthy. Specially since the
Cool, I will make some of the suggested change when I have some spare time! |
|
From going through the code of git-hub's current implementation quickly, my Is this undrstanding correct and, if so, I am under the impression that it Or am I missing something here maybe? |
|
So... 4 years have passed since the last time I saw this PR, and probably about the same amount of time since I don't use Just as a heads up, I'm not actively maintaining this project, maybe I should consider archiving it, unless someone else is willing to take over. |
|
Many thanks for the heads up!
I must confess, I am not a big fan of `gh` myself, first because it's
written in Go I am not comfortable with, second because for many things
I do not like the way it's doing things, so my idea was more to enhance
git-hub where it needs to be inhanced to cempete with `gh` but doing
things in a nicer, more polished way.
As I am not in any way a Python expert (actually quite the countrary),
and as I am rather a newcomer as far as this project's developemnt is
concerned (although using it for years and having never really switched
to GH), I'd feel scared by a maintainer hat, at least in the short run.
But, with a long enough transition period, why not.
|
|
This is more or less why I started this project, but the landscape of github/GH integration was much worse back then. I think In any case, if you want to start submitting PRs, I'll try my best to allocate some time to review them, but if the CI is not working and things gets time consuming, I'm not sure I will be able to find the time to keep up :-/ |
|
Many thanks for your openness. In case Imanage to produce something,
I'll try to get things as leighweight as possible for you, I promise.
The feature that I started to implement today is downloading logs for
GHA workflow runs. I decided to do that because it's a typical example
where I find gh's behaviour not super useful for me, i.e., displaying
everything at once. I'd prefer the logs to be just downloaded and
decompressed in a well known place andthen I'd be able to go through
them with whatever tool is convenient.
To give a bit of potentially useful context: I am completely blind so
when others find something in a web interface painful, well, it's even
worse for me. Likewise with the escape sequences gh uses in the
terminal.
|
|
Oh dear, I can't even begin to imagine how complicated it might be use tools being blind. I'm happily surprised |
|
Responding by email here, although I know the rendering of email
responses in GitHub's interface is ugly, even when the content type of
the email is set to text/markdown, sadly. Being able to _respond_ to an
existing comment, rather than just commenting, is one of the features I
find would be super useful to have in git-hub and yes, I hope to propose
a PR doing so, I am not jsut asking for features. Not to say such a Pr
wouldn't be welcome either, don't get me wrong! ;)
Leandro Lucarella (2025/02/26 01:40 -0800):
Oh dear, I can't even begin to imagine how complicated it might be use
tools being blind.
Well it gets easier with habbit and training, of course.
I'm happily surprised `git-hub` helps in any way
with that, as I didn't do anything in particular to achieve that as I
have no experience at all working with blind people.
I don't think there is really a need for experience, although it may
help on some minor things. I think, what makes git-hub so convenient is
(1) that it's CLI oriented and (2) that it's so uncluttered and devoid
of any embellishments.
In a way, for a CLI tool a rule of thumb for how blind-friendly it is
could be to make it as easy to read by scripts as possible.
That's one aspect where I find gh's CLI less friendly to use for me: it
includes escape sequences and such things with no possibility to disable
them or maybe there is one but by redirecting the output to a file, but
then you alose also the titles of the column. I think it's too bad that
these two features have to be tied together.
If there is
anything I can do to make anything more accessible, please don't
hesitate to let me know!
Thanks! I'll try to contribute myself. Supporting the contributions will
already be super helpful! :)
|
|
Oh, this is very interesting, and looking if colors could be disabled in I tried I guess it makes sense to you to have |
This command greatly simplifies setup for a previously clonned repository.
A somewhat unforturate consequence is that the name of the only
hub.hookscripthad to be changed since invoking that hook is part
setup-forknow.Since I had to rename it anyway I also renamed it to match git's own hook
naming convention of using "-"es to separate words.
I tested this a bunch (as shown with newly added examples) but do test some more
before merging.