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

string logging code sketch #12

Closed
wants to merge 3 commits into from

Conversation

wchristian
Copy link
Contributor

This is a sketch for #7

@coveralls
Copy link

Coverage Status

Coverage decreased (-6.2%) to 89.781% when pulling 162b0f7 on wchristian:bare_string_logging into bf69be8 on frioux:master.

@frioux
Copy link
Owner

frioux commented Aug 17, 2017 via email

@frioux
Copy link
Owner

frioux commented Aug 17, 2017 via email

@wchristian
Copy link
Contributor Author

Added tests, including a failing test for pre-existing code, and fixed the handling of '' or '0'. Docs coming next.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 96.324% when pulling 139c586 on wchristian:bare_string_logging into bf69be8 on frioux:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 96.324% when pulling 1b76d0b on wchristian:bare_string_logging into bf69be8 on frioux:master.

@wchristian
Copy link
Contributor Author

Alright, docs and code can be considered ready if you're ok with them. The question is what should happen with slog and slogS when given arguments. Right now they ignore them. An alternative i can think of is having them append them to the string with $text . join " ", @args.

Which of those do you prefer?

@frioux
Copy link
Owner

frioux commented Sep 21, 2017

I think I prefer how it is now, but only because it's slightly simpler.

@wchristian
Copy link
Contributor Author

Alright, then you can go ahead and pull, i think. :)

@frioux
Copy link
Owner

frioux commented Oct 28, 2017

BTW your DlogS test is wrong; the S stands for scalar, and an empty list is not a scalar. I'm probably going to just drop that test. Also the code fails the tidy test but I'll just take care of that and ensure that in the future travis runs that automatically.

@wchristian
Copy link
Contributor Author

Sure, thanks for the double-check. I must admit the tests had me getting into the woods a little. :)

As for tidy: Sorry, i'm not used to other people using it and didn't notice it when it scrolled past.

@frioux
Copy link
Owner

frioux commented Nov 24, 2017

Merged, about to release.

@frioux frioux closed this Nov 24, 2017
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.

None yet

3 participants