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

Add LTSV formatter #138

Merged
merged 1 commit into from
Jul 7, 2015
Merged

Add LTSV formatter #138

merged 1 commit into from
Jul 7, 2015

Conversation

takashi
Copy link
Contributor

@takashi takashi commented Jun 29, 2015

I added a new formatter which formats log with LTSV format

LTSV means "Labeled Tab-separated Values"
official site: http://ltsv.org/

The LTSV format focuses on access logs of web servers
and I think it is useful for logging.

@takashi
Copy link
Contributor Author

takashi commented Jun 30, 2015

format code styles and squashed commit

@pxlpnk
Copy link
Collaborator

pxlpnk commented Jun 30, 2015

Thank you for the PR 👍
The test file is missing the _spec suffix and think the tests could maybe test the actual format a bit to guarantee consistency.

@takashi
Copy link
Contributor Author

takashi commented Jul 1, 2015

@pxlpnk thank you for reviewing! fixed file name with _spec suffix and add bit spec to specify format

@pxlpnk
Copy link
Collaborator

pxlpnk commented Jul 1, 2015

Looks good. Here we would need some single quotes to make rubocop happy: https://github.com/takashi/lograge/blob/add-ltsv-formatter/spec/formatters/ltsv_spec.rb#L26

@benlovell any objections in merging this?

@takashi
Copy link
Contributor Author

takashi commented Jul 1, 2015

@pxlpnk opps, my bad. format style and squashed!

@benlovell
Copy link
Collaborator

LGTM. 👍 build failed on jruby-head though.

@takashi
Copy link
Contributor Author

takashi commented Jul 2, 2015

@benlovell thx for LGTM! but i think the reason of failure on jruby-head is different from this PR's change. WDYT?

@benlovell
Copy link
Collaborator

Yep agreed. It's unrelated.

@takashi
Copy link
Contributor Author

takashi commented Jul 2, 2015

@benlovell yep, but I cant merge without resolving this issue... ;(

@takashi takashi closed this Jul 2, 2015
@takashi takashi reopened this Jul 2, 2015
@takashi
Copy link
Contributor Author

takashi commented Jul 2, 2015

upps, mistake for closing

@benlovell
Copy link
Collaborator

Well you've triggered another build now so that's useful 😄

@pxlpnk
Copy link
Collaborator

pxlpnk commented Jul 2, 2015

I opened an issue on the jRuby repo, lets see if we get any pointers on this: jruby/jruby#3102

For the rest, I am fine with merging it and adding jruby-head again to the allowed failures.

One wish is to change the commit message to Add LTSV formatter and its spec (capitalising the a in the add) to follow our guidelines.

@takashi
Copy link
Contributor Author

takashi commented Jul 3, 2015

@pxlpnk thanks for opening issue in jruby :)

And I fixed commit message by capitalizing a in add to follow guideline.

@takashi
Copy link
Contributor Author

takashi commented Jul 6, 2015

@pxlpnk how about this PR?

@benlovell
Copy link
Collaborator

@takashi I pushed a change to master 6ccae19 that temporarily allows failures for jruby-head in the build matrix. Rebase and we'll get this merged. 👍

@takashi
Copy link
Contributor Author

takashi commented Jul 6, 2015

@benlovell okay thanks! I merged the commit that allows failure for jruby-head into this PR 👍

@benlovell
Copy link
Collaborator

@takashi I'd prefer if we didn't introduce that unnecessary merge commit. Can you reset to 71ad31b and then rebase instead please? If you need help with this, give me a shout.

@takashi
Copy link
Contributor Author

takashi commented Jul 6, 2015

@benlovell I see, and I rebased merge commit 👍 If i misunderstand your opinion, please let me know

@pxlpnk
Copy link
Collaborator

pxlpnk commented Jul 6, 2015

Rebase is perfect. A capitalized A in the add and we are ready to go! 👯
Thank you so much for your effort @takashi 💚

@takashi
Copy link
Contributor Author

takashi commented Jul 6, 2015

@pxlpnk upps, i forgot to capitalize a, and fixed it! thank you too @pxlpnk and @benlovell 🙏

@pxlpnk
Copy link
Collaborator

pxlpnk commented Jul 7, 2015

🚢

pxlpnk added a commit that referenced this pull request Jul 7, 2015
@pxlpnk pxlpnk merged commit 62f0e53 into roidrage:master Jul 7, 2015
@benlovell
Copy link
Collaborator

😍 thanks

@takashi takashi deleted the add-ltsv-formatter branch July 7, 2015 09:31
@takashi
Copy link
Contributor Author

takashi commented Jul 7, 2015

thx for merging!

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