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

Fix ${line} in text format to mean line number #270

Merged
merged 1 commit into from
Oct 20, 2017

Conversation

apiology
Copy link
Contributor

@apiology apiology commented Oct 1, 2017

How to reproduce:

Create a reasonable-sounding .pronto.yml that looks like this, based on documentation from https://github.com/prontolabs/pronto#message-format:

 text:
   format: "%{path} %{line} %{runner} %{msg}"

Run against a project

Expected results:

Receive text output that looks like this:

Rakefile 124 rubocop Use `%i` or `%I` for an array of symbols.

Observed results:

Rakefile #<struct Pronto::Git::Line line=#<Rugged::Diff::Line:70253771778400 {line_origin: :addition, content: "task quality: [:pronto, :update_bundle_audit]\n">, patch=#<struct Pronto::Git::Patch patch=#<Rugged::Patch:70253763427640>, repo=#<Pronto::Git::Repository:0x007fca7539fe08 @repo=#<Rugged::Repository:70253763428080 {path: "/Users/broz/src/vincelifedaily/.git/"}>>>, hunk=#<Rugged::Diff::Hunk:70253771778940 {header: "@@ -115,7 +115,13 @@ end\n", count: 14}>> rubocop Use `%i` or `%I` for an array of symbols.

Notes:

Looks like the json_formatter.rb already looks for line number via the same method I'm proposing here. I'm guessing changing the meaning of message.line is a nonstarter given you'd have to change all of the runners at the same time, so using the decorator to adjust the meaning seemed like the right thing.

@doomspork
Copy link
Member

@apiology this is wonderful, thank you! Would it be possible for you to update or add test coverage for this change?

@vinceatbluelabs
Copy link

@doomspork: Added - let me know what you think!

@doomspork doomspork requested a review from a team October 3, 2017 17:34
@doomspork
Copy link
Member

Thank you @apiology! I'm going to give @mmozuras an opportunity to review this change before we merge.

I'm not sure whether or not you're participating in Digital Ocean's Hacktoberfest but I went ahead and tagged the PR for you anyways 😀

@apiology
Copy link
Contributor Author

apiology commented Oct 5, 2017

Thanks--wasn't aware of it, but just signed up!

@mmozuras
Copy link
Member

@apiology thanks! 🙇

Exemplary pull request, love the fact that you've written a perfect "how to reproduce" 😄

@mmozuras mmozuras merged commit 2d61ce2 into prontolabs:master Oct 20, 2017
@mmozuras mmozuras removed their request for review October 20, 2017 18:51
apiology pushed a commit to apiology/pronto that referenced this pull request Dec 27, 2019
Fix ${line} in text format to mean line number
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.

4 participants