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 github-review #6020

Merged
merged 5 commits into from Mar 2, 2019

Conversation

Projects
None yet
5 participants
@charignon
Copy link
Contributor

commented Feb 12, 2019

Brief summary of what the package does

greview lets you submit Github code review with Emacs.

With greview-start-review you can pull the content of a pull request into a buffer as a diff with comments corresponding to the PR description. In that buffer you can add comment (global and inline) that you want to make on the pull request. Once done, you can submit these comments as a code review with one of greview-approve, greview-comment and greview-reject.

Direct link to the package repository

https://github.com/charignon/greview

Your association with the package

I am the maintainer of greview

Relevant communications with the upstream package maintainer

None needed

Checklist

Please confirm with x:

Add greview
`greview` lets you do Github code review from within Emacs.
@riscy
Copy link
Collaborator

left a comment

Great! I'd love to do code reviews inside Emacs. Here's some feedback from my end that might be useful:

  • Elisp looks clean.
  • The brief summary of what the package does you've provided in this PR would be great to flesh out your ;;; Commentary section.
  • I believe "PR URL:" (around line 340) should have a space after it, i.e. "PR URL: "
  • When I try to run this code on this PR, ghub.el is telling me: greview fails to define greview-github-token-scopes. I believe I'm on the latest version of ghub -- maybe there's something else wrong on my end?
  • Minor: greview-parse-line is overindented (Emacs [seems to prefer] to format elisp with two-space indents [starting from the left]) and inconsistent with the rest of the code
  • Some of the lines in greview-submit-review are misindented under the let*
  • Nit: GitHub is usually written with a capital H
@charignon

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2019

Thanks for the review :), I fixed the issues that you raised:

The error you ran into is probably because your GHE token isn't set up properly. Could you confirm that you followed the steps under Configuration at https://github.com/charignon/greview?

@riscy

@riscy
Copy link
Collaborator

left a comment

Testing a top-level review comment using greview. :man_in_business_suit_levitating:

@@ -0,0 +1 @@
(greview :repo "charignon/greview" :fetcher github)

This comment has been minimized.

Copy link
@riscy

riscy Feb 17, 2019

Collaborator

Experimental inline comment using greview.

@riscy

This comment has been minimized.

Copy link
Collaborator

commented Feb 17, 2019

Thanks for accommodating my fumbling. This is really nice.

I looked into the ghub authentication docs and ran the command ghub-create-token. When it asks me to define the package, I put greview and get the error: call-interactively: Symbol’s value as variable is void: greview-github-token-scopes -- so I was probably wrong to do that.

If I set the package to ghub, I get a line like machine api.github.com login riscy^ghub ... in my authoinfo which seems to work, but doesn't match the documentation in your repo.

@charignon

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

I created the token manually through the GitHub UI and added it to my authinfo file. I checked the procedure on a new host and it worked. I updated the instructions accordingly (charignon/github-review@39e8756), thanks!

@tarsius

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

From ghub's manual:

Every package should use its own token. This allows you as the author
of some package to only request access to API scopes that are actually
needed, which in turn might make it easier for users to trust your
package not to do unwanted things.

The scopes used by ‘PACKAGE’ have to be defined using the variable
‘PACKAGE-github-token-scopes’, and you have to tell ‘ghub-request’ on
behalf of which package a request is being made by passing the symbol
‘PACKAGE’ as the value of its ‘AUTH’ argument.

 (ghub-request "GET" "/user" nil :auth 'PACKAGE)

See the rest of the "Using Ghub in a Package" node for more information.

@charignon

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

Thanks for the pointer @tarsius and sorry for missing this in the first place. I added and tested the scope definition charignon/github-review@6b54cde.

@charignon

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

@riscy could you please take another look when you get a chance?

@riscy
Copy link
Collaborator

left a comment

Cool, I was able to use ghub-create-token to create a riscy^review token in my .authinfo, so it looks like that was a success -- I'm submitting this through greview. It works!

@mattiasb

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2019

One small suggestion: The "g" prefix doesn't say much. Would it make sense to call the package github-review? Or (if there are plans to support more forges): git-forge-review?

@charignon

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

@mattiasb thanks for the feedback, you are right the g prefix does not say much. I don't intend on adding support to more forges for now, renaming it to github-review is a great idea! I will work on that.

I will also rename the repo.

charignon added some commits Feb 23, 2019

@charignon

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

@riscy I renamed the package to github-review, this is ready for another round of review :) Thank you all for your work on this.

@charignon charignon changed the title Add greview Add github-review Feb 24, 2019

@riscy
Copy link
Collaborator

left a comment

Looks like it's working great. This is an exciting start for this package and I'll probably become a regular user -- a later step might be to add a minor mode to enhance using diff mode to write the reviews. If I have any nit with the latest changes, it might be to rename github-review-start-review to github-review-start for brevity.

charignon added a commit to charignon/github-review that referenced this pull request Feb 24, 2019

@charignon

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

Sure, that's a good idea for later improvements. I applied your suggestion (see charignon/github-review@f2982aa). Thanks for all the review, what is missing to get this merged?

@riscy

This comment has been minimized.

Copy link
Collaborator

commented Feb 24, 2019

I'm just giving it a first pass, I'm not a MELPA maintainer. :)

@purcell

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

Thanks everyone for the review assistance.

Nice work @charignon - this is pretty epic! Looks good to me so merging now. Welcome to MELPA! 🎉

@purcell purcell merged commit 3b57420 into melpa:master Mar 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.