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

Octopolo asks label-related questions as an array of objects #123

Merged
merged 12 commits into from
Jul 18, 2018

Conversation

emmahsax
Copy link
Contributor

@emmahsax emmahsax commented Jul 10, 2018

What and Why

No user-facing changes. Update how octopolo asks the user to update labels: although it will look the same to users right now, actually have asking a label-related question make a octopolo question object, and then ask each object. This creates the ability to ask the users multiple label-related questions for each PR or issue, instead of just a hard-coded single question.

Deploy Plan

Merge this PR and make a new release (minor version change). Put new release on RubyGems.

Rollback Plan

To roll back this change, revert the merge with git revert -m 1 MERGE_SHA and perform another deploy.

QA Plan

  • Checkout this branch of this repository
  • Run bundle install (run gem install bundler before this command if necessary)
  • Run bin/op pull-request to make a test PR using this code
    • Verify that it continues to ask you which label you wish to add
    • Verify that if you try to add multiple labels, only the first label selected will be added
  • Run bin/op issue to make a test issue using this code
    • Verify that it continues to ask you which label you wish to add
    • Verify that if you try to add multiple labels, only the first label selected will be added

@emmahsax emmahsax changed the title A user can indicate multiple labels to add at once Octopolo asks label-related questions as an array of objects Jul 16, 2018
@mpillsbury mpillsbury self-requested a review July 17, 2018 18:24
Copy link
Contributor

@mpillsbury mpillsbury left a comment

Choose a reason for hiding this comment

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

Seems like a solid refactor. QA works fine. Just the one question...

@@ -114,21 +114,18 @@ def self.perform_in(dir)

def self.ask(question, choices, skip_asking = false)
return choices.first if choices.size == 1

unless skip_asking
Copy link
Contributor

Choose a reason for hiding this comment

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

Why ignore this param now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm understanding the code correctly, we never actually call this method with the skip_asking set to true, unless it's in a test. So I'm not sure if there was a point keeping it around. Seemed to just confuse me. I could put it back in, but by the time I realized we could keep it, I had already moved around the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we take it out of the method signature then (on line 115)? I'm sure Rubocop wouldn't like us leaving it around...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right. missed that

Copy link
Contributor

@mpillsbury mpillsbury left a comment

Choose a reason for hiding this comment

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

Approved!

@production-status-check
Copy link

:octocat: Has QA approval

@emmahsax emmahsax merged commit 73e40ba into master Jul 18, 2018
@emmahsax emmahsax deleted the ask-about-approvals branch July 14, 2020 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants