Consolidate PR comments #154

doomspork · 2016-05-10T17:20:18Z

Hi @mmozuras, here are the changes I made regarding #147. I figured a PR was a good way to get and incorporate feedback. You'll see I removed the checkboxes in favor of a bulleted list.

mmozuras · 2016-05-15T18:03:31Z

@doomspork good work! 😄 🙇

@doomspork @kt0 what do you think about a separate runner?
I'm just not sure if consolidation should be the default. And having a separate runner seems cleaner than a configuration option.

doomspork · 2016-05-15T18:05:55Z

@mmozuras a separate runner or a configuration option works for me. I'd like to look into the BitBucket runner and see how much of this can be shared.

mmozuras · 2016-05-15T18:08:23Z

@doomspork if you'd find a general solution that works for GitHub/Bitbucket/Gitlab, it would amazing 😄

doomspork · 2016-05-15T18:16:39Z

After a quick glance I don't think that should be a problem. What do you think @mmozuras, all new runners or update the existing runners to support a consolidated option?

mmozuras · 2016-05-15T18:22:41Z

@doomspork on the second thought, having a separate runner would be too confusing. Runners should only be about "where/how to show/post". So, do the config option 😄

doomspork · 2016-05-15T18:54:28Z

@mmozuras take a peek when you have a moment. I pulled most of the PR code into a separate PullRequestFormatter class which I inherit from in both BitBucket and GitHub. Tests are passing.

It doesn't look like GitLab has a PR formatter yet?

doomspork · 2016-05-15T18:56:18Z

lib/pronto/formatter/pull_request_formatter.rb

@@ -0,0 +1,87 @@
+module Pronto
+ module Formatter
+ class PullRequestFormatter


To clear up any confusion maybe this should be something like: PullRequestFormatterBase or AbstractPullRequestFormatter.

Since this code can almost entirely be re-used for the commit formatters maybe this would be better as: GitClientFormatterBase or some variation of that. Thoughts?

doomspork · 2016-05-15T19:04:08Z

I think I can further consolidate GitHub, GitLab, and BitBucket commit formatters and incorporate the consolidation. Want me to go ahead with that @mmozuras?

doomspork · 2016-05-15T19:04:43Z

lib/pronto/formatter/pull_request_formatter.rb

+ def submit_comments(client, comments)
+ puts comments
+ comments.each { |comment| client.create_pull_comment(comment) }
+ rescue Octokit::UnprocessableEntity => e


Is there a BitBucket client error we should listen for? The BitBucket formatter didn't handle any exceptions.

@doomspork BitbucketClient uses httparty. We could listen for HTTParty::Error.

doomspork · 2016-05-22T19:27:46Z

@mmozuras just a heads up it may be a week or two before I finish up this change, I'm in the process of moving across the country. 😄

mmozuras · 2016-05-24T12:08:11Z

@doomspork no worries, it's an amazing feature - well worth the wait 😄

doomspork · 2016-06-13T17:40:04Z

@mmozuras I'm finally settled in, I'll resume my work on this now 👍

doomspork · 2016-06-20T18:32:55Z

@mmozuras I've added the consolidation portion to commits and PRs, mind taking a look at these changes now?

mmozuras · 2016-06-20T19:57:48Z

lib/pronto/formatter/commit_formatter.rb

+ end
+
+ def line_number(message)
+ message.line.new_linen


new_linen - a mistype perchance? 😄

Good catch 👍

mmozuras · 2016-06-20T20:06:05Z

@doomspork amazing! 🙇

I've also found a couple of style issues, which revealed that TravisCI no longer runs Pronto successfully (I'll fix that, no worries). Could you run pronto with rubocop locally on your PR and fix the issues? 😸

doomspork · 2016-06-20T20:21:47Z

@mmozuras I cleaned up the pieces of feedback you left, ran Rubocop, and resolved the issues with my code. There's still a number of outstanding Rubocop changes but I assume you're not expecting me to fix them all 😀

doomspork · 2016-06-20T20:30:37Z

@mmozuras I added a second commit with a number of Rubocop fixes for other pieces of code, I can drop the commit if you'd prefer 😀

doomspork · 2016-06-27T22:08:19Z

@mmozuras any other feedback or things you'd like to see changed?

mmozuras · 2016-07-03T16:10:50Z

lib/pronto/cli.rb

@@ -6,7 +6,7 @@ class CLI < Thor
 require 'pronto/version'

 class << self
- def is_thor_reserved_word?(word, type)
+ def thor_reserved_word?(word, type)


@doomspork overrides a method in thor. Can't be changed, should be reverted 😄

mmozuras · 2016-07-03T16:12:08Z

@doomspork only change that one line (the last comment) and it's good to go! 😄

mmozuras · 2016-07-03T16:38:33Z

lib/pronto.rb

@@ -29,6 +29,9 @@
 require 'pronto/formatter/colorizable'
 require 'pronto/formatter/text_formatter'
 require 'pronto/formatter/json_formatter'
+require 'pronto/formatter/git_formatter_base'


Actually, I suggest removing base from the name too. Not using that convention anywhere else in this project.

doomspork · 2016-07-03T21:51:01Z

@mmozuras feedback has been incorporated 👍

mmozuras · 2016-07-04T19:00:11Z

@doomspork the code looks good 👍

Alas, Travis is still failing at after_success: https://travis-ci.org/mmozuras/pronto/jobs/142075404. If you could try to figure it out, it would be amazing 😄. If you won't find the time, I'll do it on Wednesday. It'll be a holiday here in Lithuania, so I will also release the next version of Pronto on that magical day (including this PR) 😄.

doomspork · 2016-07-10T17:11:19Z

Thanks @mmozuras! I've been busy preparing for a conference this week so I haven't had a chance to investigate the after_success further.

mmozuras · 2016-07-10T17:37:11Z

@doomspork no, thanks to you for contributing 😄

What kind of conference? Are you going to speak?

doomspork · 2016-07-10T18:36:30Z

@mmozuras you're welcome! I'm working on Pronto-as-a-Service for work, once it's ready I'll open-source it and give you a ping.

I am speaking and doing a workshop @ http:https://elixircamp.io. If you happen to be in NYC stop by! 😀

mmozuras · 2016-07-10T18:55:52Z

@doomspork looks like a fun conference! Alas, I won't be in NYC, but good luck on your talk and workshop 😄

I'm working on Pronto-as-a-Service for work, once it's ready I'll open-source it and give you a ping.

Exciting! Will it be free for OSS and paid for everyone else? Looking forward to see it 😄

jhass · 2016-08-07T17:33:24Z

lib/pronto/config.rb

@@ -11,6 +11,10 @@ def initialize(config_hash = ConfigFile.new.to_h)
 end
 end

+ def consolidate_comments?
+ !@config_hash['consolidate_comments'].nil?


This bypasses the ability to set it via an environment variable, right?

@jhass correct, ENVs are not currently supported for: consolidate_comments, verbose, max_warnings, or exclusions.

I think we should consider changing that.

Meanwhile https://github.com/mmozuras/pronto#configuration should be updated I guess, for both, mentioning that there are no overrides as well as this new option.

That's up to @mmozuras to decide. 😀

Happy to contribute the change if it's something he's keen on.

@doomspork @jhass having consistency between config yml and ENV config would be amazing. I'd gladly accept a PR that improves it.

!@config_hash['consolidate_comments'].nil?
I found this a little bit counter intuitive.

My .pronto.yml config:

consolidate_comments: false

as a result !false.nil? is truthy.

Good catch @mknapik, I've opened a PR with a fix.

Please see #187

doomspork · 2016-10-18T18:17:50Z

@mmozuras we just opened sourced our Pronto-as-a-Service that I mentioned to you, feel free to take a peek: https://github.com/wombatsecurity/nag

Any and all feedback is welcomed 👍

mmozuras · 2016-10-18T19:54:21Z

@doomspork thanks, I'll try to find the time to try it myself in the next couple of days 😄

doomspork · 2016-10-18T20:01:46Z

I need to update the README a bit but feel free to reach out to me via the email in my profile if you need a hand 😀

doomspork force-pushed the consolidated-messaging branch from d0a8ea0 to df91c88 Compare May 11, 2016 00:58

doomspork mentioned this pull request May 13, 2016

Is there any way to combine errors of same line in one comment? #147

Closed

doomspork force-pushed the consolidated-messaging branch from d3d9177 to 4027997 Compare May 15, 2016 18:54

doomspork reviewed May 15, 2016
View reviewed changes

doomspork mentioned this pull request Jun 13, 2016

Add more detail to GitHub messages #159

Closed

doomspork force-pushed the consolidated-messaging branch from 22b0e42 to 0e6ec5c Compare June 20, 2016 18:32

mmozuras mentioned this pull request Jun 20, 2016

Don't see any status on Github #155

Closed

mmozuras reviewed Jun 20, 2016
View reviewed changes

doomspork force-pushed the consolidated-messaging branch from ac88bdf to b7e0997 Compare June 20, 2016 20:21

mmozuras reviewed Jul 3, 2016
View reviewed changes

mmozuras mentioned this pull request Jul 3, 2016

Pronto after_success step fails #163

Closed

doomspork force-pushed the consolidated-messaging branch from 68b44ee to 2144cfd Compare July 3, 2016 21:49

doomspork added 2 commits July 3, 2016 15:50

Consolidate commit and PR comments

135a240

Cleanup rubocop errors

2488e6a

doomspork force-pushed the consolidated-messaging branch from 2144cfd to 2488e6a Compare July 3, 2016 21:50

mmozuras merged commit a41eb03 into prontolabs:master Jul 10, 2016

doomspork deleted the consolidated-messaging branch July 10, 2016 17:11

doomspork restored the consolidated-messaging branch August 1, 2016 20:56

jhass reviewed Aug 7, 2016
View reviewed changes

doomspork deleted the consolidated-messaging branch October 5, 2016 23:04

doomspork mentioned this pull request Oct 5, 2016

Support ENV for all config options #181

Merged

doomspork mentioned this pull request Oct 18, 2016

Allow for nil and false values for consolidation #187

Merged

Consolidate PR comments #154

Consolidate PR comments #154

Conversation

doomspork commented May 10, 2016

mmozuras commented May 15, 2016 • edited Loading

doomspork commented May 15, 2016

mmozuras commented May 15, 2016

doomspork commented May 15, 2016

mmozuras commented May 15, 2016

doomspork commented May 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doomspork commented May 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doomspork commented May 22, 2016

mmozuras commented May 24, 2016

doomspork commented Jun 13, 2016

doomspork commented Jun 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmozuras commented Jun 20, 2016 • edited Loading

doomspork commented Jun 20, 2016

doomspork commented Jun 20, 2016

doomspork commented Jun 27, 2016

Choose a reason for hiding this comment

mmozuras commented Jul 3, 2016 • edited Loading

Choose a reason for hiding this comment

doomspork commented Jul 3, 2016

mmozuras commented Jul 4, 2016

doomspork commented Jul 10, 2016

mmozuras commented Jul 10, 2016

doomspork commented Jul 10, 2016 • edited Loading

mmozuras commented Jul 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doomspork commented Oct 18, 2016

mmozuras commented Oct 18, 2016

doomspork commented Oct 18, 2016

mmozuras commented May 15, 2016 •

edited

Loading

mmozuras commented Jun 20, 2016 •

edited

Loading

mmozuras commented Jul 3, 2016 •

edited

Loading

doomspork commented Jul 10, 2016 •

edited

Loading