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

Add a new subcommand git pull-request info #1109

Closed
wants to merge 2 commits into from

Conversation

pcorpet
Copy link
Contributor

@pcorpet pcorpet commented Feb 11, 2016

I had to update the vendored octokit. I strongly recommend reviewing each commit separately as one is just about updating and the other one about adding the new feature.

@pcorpet pcorpet force-pushed the pull-request-info branch 4 times, most recently from 5de188b to 580a627 Compare February 12, 2016 08:47
@mislav
Copy link
Owner

mislav commented Feb 15, 2016

Hi thanks for the big PR, but you didn't explain much about how you would use this feature.

I'm going to reject this PR for now not because it's a bad idea, but because it's not the right time to make the implementation for this yet. Let me elaborate:

  1. I'd like us to focus on hub issue first, through Revamp hub issue commands #1099. It would be great if the output of issue listing is configurable through --pretty flag such as git-log.
  2. I don't want to be using go-octokit further. In fact, I'm switching new implementations within hub to a lightweight client that we internally call simpleApi. That's why I'm 👎 on the octokit update.
  3. Next, I'd like us to implement hub issue show that displays information about a single issue, plus lists all comments. The format for this should probably also be configurable via the same mechanism.
  4. After all that is done, let's take everything that we learned/implemented from that and apply to a new hub pr command that lists all PRs and also allows to fetch information for a specific PR.

I hope that makes sense! I'd appreciate your feedback.

@mislav mislav closed this Feb 15, 2016
@mislav mislav added the feature label Feb 15, 2016
@pcorpet
Copy link
Contributor Author

pcorpet commented Feb 15, 2016

A little more explanation on how I would use this feature:

We have a workflow where we use a custom command to merge PR (well actually rebase them on master). This workflow is using a command line (git submit, a custom command for us) that checks multiple things, for instance that the PR is correctly rebased as only one commit on top of master, and others. I wanted to check before submitting that a branch has indeed a PR open on GitHub and that it got at least one "LGTM" comment from someone else than the author. That would make sure that the code has actually been reviewed.

I can see why you're rejecting this PR for now, and I'm willing to contribute (I am between jobs at the moment) to make my workflow work soon.

  1. I understand you want to focus on hub issue. How can I help? Do you want me to review your code? take some of the TODO as a separate task?
  2. ok, I saw simpleApi, that seems fine to me.

@mislav
Copy link
Owner

mislav commented Feb 15, 2016

  1. I understand you want to focus on hub issue. How can I help?

I'm glad you asked. Reviewing the PR (the command API, feature-set, docs, implementation) would be great feedback for me. Next, I might need help with the following features, probably best done as separate PRs:

  • I would like to provide configurability of output via --format parameter a la format:<string> within git-log(1). The tricky formatting features, apart from the value placeholders, that we can consider implementing are:

       o   %w([<w>[,<i1>[,<i2>]]]): switch line wrapping, like the -w option of git-shortlog(1).
    
       o   %<(<N>[,trunc|ltrunc|mtrunc]): make the next placeholder take at least N columns, padding spaces on the right if necessary. Optionally truncate at
           the beginning (ltrunc), the middle (mtrunc) or the end (trunc) if the output is longer than N columns. Note that truncating only works correctly
           with N >= 2.
    
       o   %<|(<N>): make the next placeholder take at least until Nth columns, padding spaces on the right if necessary
    
       o   %>(<N>), %>|(<N>): similar to %<(<N>), %<|(<N>) respectively, but padding spaces on the left
    
       o   %>>(<N>), %>>|(<N>): similar to %>(<N>), %>|(<N>) respectively, except that if the next placeholder takes more spaces than given and there are
           spaces on its left, use those spaces
    
       o   %><(<N>), %><|(<N>): similar to % <(<N>), %<|(<N>) respectively, but padding both sides (i.e. the text is centered)
    
  • The git issue show <N> feature would have to be implemented.

  • Issue updating, such as closing/reopening an issue, adding labels or a comment, etc. would have to be implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants