Add option --milestone to open_issue command #95
Conversation
|
My tests are not failing locally, however I am using |
|
I just tested this on a fresh pull from master, changed the README, and then ran it through Travis. It fails https://travis-ci.org/tarebyte/teachers_pet/jobs/40156129 |
| @@ -6,6 +6,7 @@ class Cli | |||
| option :title, desc: "The title of the issue to be created" | |||
| option :body, banner: 'PATH', desc: "The path to the file containing the issue body (.txt or .md)" | |||
| option :labels, banner: 'LABEL1,LABEL2' | |||
| option :milestone, desc: "The milestone you would like to associate with the issue", type: :numeric | |||
There was a problem hiding this comment.
For some reason the CI breaks when all I add is this option
https://travis-ci.org/tarebyte/teachers_pet/jobs/40206454
Update: I created a bug elsewhere and didn't realize it. This is no longer true
There was a problem hiding this comment.
Where would someone find the milestone "number"? Doesn't seem that it's reflected in the URL, e.g. https://github.com/scala/scala/milestones/on-hold.
There was a problem hiding this comment.
I don't see anywhere in the UI or the URL where the milestones are listed, maybe I'm missing something.
There was a problem hiding this comment.
I was trying to move away from prompts, but one solution would be that if they include --milestone without a value, it could list the milestones for the repository and ask them to choose which to use.
This leads to another question, which is how would the milestones get created across the repositories in the first place. Do we need an create_milestone command? This feels like 🐢s all the way down, and while I appreciate all the work you've put into this, I'm questioning the value relative to this extra required effort.
There was a problem hiding this comment.
It's not a problem, if nothing else I had good learning experience out of it, and I can extract out the json code I wrote and make that into a new PR 😃.
I don't know how often educators use milestones in students repos. My guess would be not very often. Just by talking to educators the last few days it seems like it's a very rare case.
| request_stubs << stub_request(:post, "https://testteacher:abc123@api.github.com/repos/testorg/#{username}-testrepo/issues"). | ||
| with(body: "{\"labels\":#{labels_list},\"title\":\"#{issue_title}\",\"body\":\"#{issue_body}\"}"). | ||
| with(body: { |
There was a problem hiding this comment.
This seems a lot better to me than trying to type out the raw string yourself.
This convention is also being used in create_student_teams_spec.rb
|
@afeld my specs pass on rubies Any idea why? |
|
Figured it out, I wasn't looking at the error logs correctly. Also the reason this is now an issue is because web mock merged in this PR bblimke/webmock#427 They started asserting the body of the response, which cannot be a hash. |
| @@ -42,6 +43,13 @@ def create | |||
| next | |||
| end | |||
|
|
|||
| if @issue[:options][:milestone].nil? | |||
| if self.client.milestone("#{@organization}/#{repo_name}", @issue[:options][:milestone]).nil? | |||
There was a problem hiding this comment.
A little hard to understand what these conditionals are doing... mind adding inline comments, or (better yet) moving them into a method with a descriptive name?
There was a problem hiding this comment.
I agree it is a convoluted.
The code I have actually won't work because it runs the conditional only if the milestone is nil.
What do you think about something like this?
# If the a milestone is an option, make sure the milestone is on GitHub
if @issue[:options][:milestone]
unless milestone_exists_on_github?(self.client, "#{@organization}/#{repo_name}", @issue[:options][:milestone])
puts " --> Milestone not found, skipping '#{repo_name}'"
next
end
end
def milestone_exists_on_github?(client, repo, milestone)
!client.milestone(repo, milestone).nil?
end
|
Sorry for the slow response – just a couple minor comments above. Thanks! |
| @@ -58,5 +58,13 @@ def add_users_to_team(organization, team, usernames) | |||
| end | |||
| end | |||
| end | |||
|
|
|||
| def milestone?(organization, repo, milestone) | |||
There was a problem hiding this comment.
I missed the pattern in the client_decorator, a milestone? method seems to best fit the pattern for methods like these.
There was a problem hiding this comment.
From https://github.com/bbatsov/ruby-style-guide:
The names of predicate methods (methods that return a boolean value) should end in a question mark.
👍
|
@afeld does this look good to merge? |
|
#95 (comment) seems like a pretty big problem, unfortunately. |
|
Shoot I did not see that comment, that's a very good point |
|
Going to close, but am happy to reopen if we can figure out a reasonable way to do it. Thanks @tarebyte! |
Closes #86, wanted to get an opinion on this before I finished up the feature with tests.