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

new functionality for review #99

Merged
merged 15 commits into from
May 15, 2015
Merged

Conversation

meesterdude
Copy link
Contributor

I find that using a reflow a lot, i lose track of the PR's it creates, and have a hard time picking up where i left off.

This makes several changes:

  • like git commit, if you do not pass in the message (or title, in this case) it will prompt you with an editor; first line is the title, rest is the body. This is a change of the default behavior of the review command, but it's more in line with what I'd expect to happen.
  • if you pass the title and/or the body options to review, it will use that and default to the branch/last commit, if you left either off.
  • updates the default title to be the branch name, default body to be the last commit
  • prints the PR on the command line to look over
  • prompts user before submitting PR; default is yes (enter)
  • adds an -e flag that allows you to edit in the editor, prefilled with what the default message would be (branch title, last commit body)
  • updates README to reflect new workflow
  • bumps version to 0.6.0 per major.minor.bugfix methodology

end

if editor
system('nano', commit_msg_file)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use $EDITOR if it's set, cause nano would make me lose my mind

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would love to - but i couldn't find a solution that worked. got an example i can run with?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$EDITOR is pretty standard, and we're only supporting OS X and Ubuntu for now. Also, you'll want to take advantage of GitReflow.run for running any system commands (see ref). You can accomplish this with GitReflow.run("$EDITOR #{commit_msg_file}")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codenamev i tried the code you mentioned - it just hangs, can't even ctrl+C, have to kill -9 in another terminal. I think its around sourcing the environment variables. Maybe we default to nano, and allow cfg to set preference? nano is the least controversial as a default

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right, I forgot you need GitReflow.run("$EDITOR #{commit_msg_file}", with_system: true) to run it as system instead of backticks

@nhance
Copy link
Member

nhance commented May 14, 2015

I really like this


review_options['title'] ||= GitReflow.get_first_commit_message
review_options['body'] ||= review_options['title']
commit_msg_file = '/tmp/git_reflow_pr_msg'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can guarantee that every system will have a /tmp directory. can we use the .git directory and create another file like git does with the commit messages? maybe .git/GIT_REFLOW_PR_MSG

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the naming here is throwing me too after reading more code below. let's rename commit_msg_file to pull_request_msg_file

@codenamev
Copy link
Collaborator

yeah, HUGE 👍 from me on this too 😸 great work!

puts "Body:\n#{review_options['body']}\n"
puts "--------\n"
puts "Submit PR? (Y/n):"
exit 0 if STDIN.gets.chop.downcase =~ /n/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're using the highline gem for input: GitReflow.review review_options if ask("Submit pull request? (y/n)") =~ /y/i

uses ask instead of custom prompting
renames variable commit_msg_file to pull_request_msg_file
@meesterdude
Copy link
Contributor Author

ok, updated this.

I addressed most of val's concerns. One of them, the use_editor idea i tried but found the code to be less clear, so i undid.

  • I removed the -e flag, it'll always provide the branch name as the title, at least as a starting point; i will be confused with a blank editor.
  • simplified some of the code.
  • removed extra space that might appear between title and body
  • updated the prompt to be clearer what the default will be with enter key

'title' => (global_options[:title] || GitReflow.current_branch),
'body' => global_options[:message]
}
editor = false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it still seems strange to have this here... why can't we just change line 22 into an else?

@meesterdude
Copy link
Contributor Author

@codenamev updated with your suggestions

@codenamev
Copy link
Collaborator

LGTM

@nhance
Copy link
Member

nhance commented May 15, 2015

lgtm

@meesterdude
Copy link
Contributor Author

thanks lads!

meesterdude added a commit that referenced this pull request May 15, 2015
review command now prompts with editor if title or body options are not provided
@meesterdude meesterdude merged commit a081644 into reenhanced:master May 15, 2015
@meesterdude meesterdude deleted the rj_test branch May 15, 2015 18:11
@meesterdude meesterdude restored the rj_test branch May 15, 2015 18:25
meesterdude added a commit that referenced this pull request May 15, 2015
LGTM given by: @codenamev, @nhance
Closes #99

Squashed commit of the following:

commit c5d7be7
Author: Russell Jennings <[email protected]>
Date:   Fri May 15 13:51:46 2015 -0400

      bumps version to 0.6.0

commit 4fa4e75
Author: Russell Jennings <[email protected]>
Date:   Fri May 15 13:44:39 2015 -0400

      updates review command logic, and updates to use git root directory

commit 92ef82c
Author: Russell Jennings <[email protected]>
Date:   Fri May 15 13:43:59 2015 -0400

      updates readme to remove -e documentation, no longer valid

commit d7ef891
Author: Russell Jennings <[email protected]>
Date:   Thu May 14 17:01:34 2015 -0400

      changes prompt question to only show what enter will do; clearer behavior, and the opposite is inferred

commit 0fa557f
Author: Russell Jennings <[email protected]>
Date:   Thu May 14 17:00:07 2015 -0400

      updates to use whatever the user has defined as

commit f9882df
Author: Russell Jennings <[email protected]>
Date:   Thu May 14 15:49:28 2015 -0400

      [cmd] updates code per PR
    uses ask instead of custom prompting
    renames variable commit_msg_file to pull_request_msg_file

commit 3c11bdd
Author: Russell Jennings <[email protected]>
Date:   Wed May 13 19:51:42 2015 -0400

      [lib] expands on review functionality and adds ability to exit

commit 727594b
Author: Russell Jennings <[email protected]>
Date:   Wed May 13 19:50:53 2015 -0400

      [doc] updatyes README with new workflow info

commit 970c73c
Author: Russell Jennings <[email protected]>
Date:   Wed May 13 19:15:35 2015 -0400

      [lib] updates review command

commit 48c60ea
Author: Russell Jennings <[email protected]>
Date:   Wed May 13 18:54:18 2015 -0400

      [lib] fixes missing end

commit ea9b522
Author: Russell Jennings <[email protected]>
Date:   Wed May 13 18:51:54 2015 -0400

      [lib] updates review.rb to open editor and adds -e option to edit the message that would normally be commited

commit be491dc
Author: Russell Jennings <[email protected]>
Date:   Wed May 13 18:51:18 2015 -0400

      [version] bumps from 0.5.2 to 0.6.0 per major.minor.bugfix methodology
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.

3 participants