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

Fixing whitespaces errors #51

Merged
merged 1 commit into from
May 29, 2015
Merged

Fixing whitespaces errors #51

merged 1 commit into from
May 29, 2015

Conversation

bessarabov
Copy link
Contributor

I've used script whiter examples lib xt t from the module https://metacpan.org/module/Test::Whitespaces to fix all problems with whitespaces.

Here is the list of rules that Test::Whitespaces uses:

  • Each line ends with "\n" (including the last line)
  • For new lines "\n" is used (not "\r\n")
  • There are no ending spaces on the lines
  • 4 spaces are used instead of tabs
  • No empty lines in the end of file

@bessarabov
Copy link
Contributor Author

I'm glad that you have Travic CI conected to this repo. Because of travis I found out that after this change one test fails. I've used Test::Differences to find out the details:

bessarabov@b-yanote:~/git/Data-Printer$ prove t/01-p.t 
t/01-p.t .. 1/? 
#   Failed test 'creepy regex'
#   at t/01-p.t line 278.
# +---+-------------------+-------------------+
# | Ln|Got                |Expected           |
# +---+-------------------+-------------------+
# *  1|'\\\\\s\n          |'\\\\\n            *
# |  2|      |            |      |            |
# |  3|    ^ \\s* go \\s  |    ^ \\s* go \\s  |
# |  4|  (modifiers: x)'  |  (modifiers: x)'  |
# +---+-------------------+-------------------+
# Looks like you failed 1 test of 40.
t/01-p.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/40 subtests 

Test Summary Report
-------------------
t/01-p.t (Wstat: 256 Tests: 40 Failed: 1)
  Failed test:  36
  Non-zero exit status: 1
Files=1, Tests=40,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.08 cusr  0.02 csys =  0.12 CPU)
Result: FAIL

I found out that this behavior goes from this lines in lib/Data/Printer.pm:

sub REF {
    my ($item, $p) = @_;
    my $string = '';

    # look-ahead, add a '\' only if it's not an object
    if (my $ref_ahead = ref $$item ) {
        $string .= '\\ ' if grep { $_ eq $ref_ahead }                                      ### <--- This line
            qw(SCALAR CODE Regexp ARRAY HASH GLOB REF);
    }  

But it change \\ to \\ it breaks a lot of other tests: https://travis-ci.org/bessarabov/Data-Printer/jobs/9060074

There are 3 ways of solving this problem:

  • decline this pull request and leave everything as it it
  • revert file t/01-p.t and leave whitespace in the end of line (and also add ignore rule to the test Testin that source code has no whitespaces errors #52)
  • patch code so p() will not return spaces in the end of lines

My idea that the variant 3 should be implemented (it is the hardiest one, but it is the most correct way of solving this issue).

What do you think?

@garu
Copy link
Owner

garu commented May 29, 2015

Nice! Actually, while I agree with you that variant 3 should be implemented, the issue I think is mostly because the regex being tested begins with a newline, and this is the only reason why the space becomes irrelevant. Considering that "relevant trailing whitespace is evil" I will fiddle with the expected string so the tests (and #52) remain valid, without needing the overhead of changing p() just because we want to remove trailing whitespace :)

Thanks again for the patch! (and for all the others)

garu added a commit that referenced this pull request May 29, 2015
@garu garu merged commit 10c2548 into garu:master May 29, 2015
@garu
Copy link
Owner

garu commented May 29, 2015

Done :) b0879b8

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.

2 participants