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

better logging. support for multiple lines. #57

Merged
merged 4 commits into from
Nov 8, 2016
Merged

better logging. support for multiple lines. #57

merged 4 commits into from
Nov 8, 2016

Conversation

mstreuhofer
Copy link
Collaborator

No description provided.

@zbeekman
Copy link
Collaborator

zbeekman commented Nov 6, 2016

Can I ask you to please push another commit adding a comment with an example multi-line output?

Other than that, LGTM 👍

I'll merge this after a couple of days in the event we don't hear from @kvz before then.

@mstreuhofer
Copy link
Collaborator Author

alright. now i don't know anymore. i don't have any knowledge about how these tests work and why they would fail still.

@zbeekman
Copy link
Collaborator

zbeekman commented Nov 6, 2016

alright. now i don't know anymore. i don't have any knowledge about how these tests work and why they would fail still.

Yes, I am a bit flummoxed by the failures as well. My best guess is that this is a line ending issue. I'll dig into it a bit.

Despite the test failures, I think this PR looks great and is good to go as soon as the testing issue is resolved.

@zbeekman
Copy link
Collaborator

zbeekman commented Nov 6, 2016

I think this is how to save the fixtures... I'm happy to do it: #3 (comment)

I'll drop your last few commits when I merge just FYI

@mstreuhofer
Copy link
Collaborator Author

mstreuhofer commented Nov 6, 2016

fixed it. it was my fault really. my own code (2920b13#diff-0e14a25824529e9df0c89de977ddd046R66) adds a space at the end of the line so empty lines in between don't get dropped while splitting later.

added the missing space at the end of the line to the test files.

@zbeekman
Copy link
Collaborator

zbeekman commented Nov 6, 2016

Could you please squash 81bd57e, 719b976, 06cea98 and bc19e33?

fixed it. it was my fault really. my own code (https://github.com/kvz/bash3boilerplate/pull/57/files#diff-0e14a25824529e9df0c89de977ddd046R66) adds a space at the end of the line so empty lines in between don't get dropped while splitting later.

Speaking of which, do you have any ideas on how to accomplish this without adding non-printing white space at line endings? Many people (myself included) discourage the practice since it can lead to the sort of confusion we just witnessed. Can you think of any alternative implementations that wouldn't add whitespace to blank lines/line endings?

I don't think this is a show stopper for merging your PR, but may catch people by surprise if they are then trying to parse or process the script output.

@mstreuhofer
Copy link
Collaborator Author

i found another way to do the multiline logging. cleaner and easier too. so that's good. no more secretly added spaces.

regarding the sqashing of commits: i don't know how this would be done on my side but i read that the receiving end of a PR in GitHub can do exactly that with the push of a button.

@zbeekman
Copy link
Collaborator

zbeekman commented Nov 7, 2016

Great news about the more elegant solution! I can do the squashing for you.
Essentially you need to do git rebase -i <commit-before-those-to-be-squashed> . After that git fires up an editor
with a list of commits between the current HEAD and the commit specified
on the command line. Mark the ones you want to squash with squash or just
"s".

Anyway don't worry about it. Cheers
On Sun, Nov 6, 2016 at 6:53 PM Manuel Streuhofer [email protected]
wrote:

i found another way to do the multiline logging. cleaner and easier too.
so that's good. no more secretly added spaces.

regarding the sqashing of commits: i don't know how this would be done on
my side but i read that the receiving end of a PR in GitHub can do exactly
that with the push of a button.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#57 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAREPDpjS7dIZuLwu_DXOUcLoMHSzNciks5q7mhrgaJpZM4KqVqd
.

@mstreuhofer
Copy link
Collaborator Author

squashed the commits as requested

@kvz kvz merged commit f82bbe5 into kvz:master Nov 8, 2016
@kvz
Copy link
Owner

kvz commented Nov 8, 2016

I just merged this, thanks for all the hard work @mstreuhofer!

@kvz
Copy link
Owner

kvz commented Nov 8, 2016

Just released v2.1.0 that contains this work

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