Skip to content

Please consider: configurable newline character#2

Merged
gisle merged 1 commit into
gisle:masterfrom
perlancar:master
Feb 11, 2012
Merged

Please consider: configurable newline character#2
gisle merged 1 commit into
gisle:masterfrom
perlancar:master

Conversation

@perlancar

Copy link
Copy Markdown

I'm maintaining Data::Dump::OneLine, but it will be simpler if Data::Dump single-line dumping builtin.

@gisle

gisle commented Feb 11, 2012

Copy link
Copy Markdown
Owner

I'm positive to applying this after the following issues has been addressed:

  • I don't think you have sorted out all interactions with $INDENT and proper spacing. Look at the refcnt and require cases. Check what happens for tied stuff as well.
  • I don't think setting $NL to "\n\n" is a valid use case. Don't suggest it is (in the docs)
  • Need to document that comments will not show up when $NL is enabled
  • Need to test comment, refcnt, require, tied behavior

@gisle
gisle merged commit e1341b8 into gisle:master Feb 11, 2012
@gisle

gisle commented Feb 11, 2012

Copy link
Copy Markdown
Owner

It says that I merged it, but that was just a mistake. My master doesn't contain this patch any more. Don't understand how I can reopen this request, so unless you figure it out consider creating a new pull request.

@perlancar

Copy link
Copy Markdown
Author

I'll look over at the issues.

Some questions:

  1. Can I use Test::More? corelist says Test::More is bundled since 5.6.2. But I'm perfectly ok with Test or printing TAP manually.
  2. I agree that $NL = "\n\n" is not a valid use case, so any suggestion on a better name for $NL? Or keep it as $NL.
  3. What do you think about configurable right margin? There are a couple of hardcoded values like 60 in the code.

@gisle

gisle commented Feb 14, 2012

Copy link
Copy Markdown
Owner

On 14. feb. 2012, at 11:36, Steven Haryanto wrote:

I'll look over at the issues.

Some questions:

  1. Can I use Test::More? corelist says Test::More is bundled since 5.6.2. But I'm perfectly ok with Test or printing TAP manually.

Feel free to use Test::More for new tests where you find use for it. I just like Test better for trivial stuff.

  1. I agree that $NL = "\n\n" is not a valid use case, so any suggestion on a better name for $NL? Or keep it as $NL.

Just keep it $NL. But there is no need suggesting setting it to anything else than "" or "\n". Perhaps "\r\n" is make sense is some cases, but I don't think you should mention that either.

  1. What do you think about configurable right margin? There are a couple of hardcoded values like 60 in the code.

It's not really a right margin as deep stuff happily overflow it. If you make it configurable it ought to be documented and it's hard to explain exactly what it is :-)

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