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

set term_width to 80 #8

Merged
merged 2 commits into from Dec 3, 2016

Conversation

Projects
None yet
3 participants
@Varadinsky
Contributor

Varadinsky commented Nov 29, 2016

CPAN PRC :-)

This should prevent unit test from depending on system's term width:

http://www.cpantesters.org/cpan/report/17e6f288-b3d6-11e6-b68e-b4ce77eefe28

dolore magna- | | al...

ok( $text =~ /magna al\.\.\./, 'text is cut off at 255 chars' );

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Nov 29, 2016

Owner

Hi @Varadinsky,

Thanks for this. :) This code change would consistently set the term_width to 80, regardless of the logic in LWP::ConsoleLogger::_build_term_width(). I think that's a pretty big hammer to use here.

It looks to me like this is at least partially a problem with the test. Because of the width of the terminal, there's a hyphen being inserted at magna- which the test does not allow for.

@andyjack I believe you wrote the test. Thoughts on how best to handle this?

Owner

oalders commented Nov 29, 2016

Hi @Varadinsky,

Thanks for this. :) This code change would consistently set the term_width to 80, regardless of the logic in LWP::ConsoleLogger::_build_term_width(). I think that's a pretty big hammer to use here.

It looks to me like this is at least partially a problem with the test. Because of the width of the terminal, there's a hyphen being inserted at magna- which the test does not allow for.

@andyjack I believe you wrote the test. Thoughts on how best to handle this?

@andyjack

This comment has been minimized.

Show comment
Hide comment
@andyjack

andyjack Nov 29, 2016

Contributor

I think trying to reverse the formatting of the output with a regex, to remove the hyphen and intervening whitespace and | would be OK. A similar thing is already done for the XML test.

Contributor

andyjack commented Nov 29, 2016

I think trying to reverse the formatting of the output with a regex, to remove the hyphen and intervening whitespace and | would be OK. A similar thing is already done for the XML test.

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Nov 30, 2016

Owner

@Varadinsky would you be able to rework your pull request to move the fix from the module to the tests as discussed above?

Owner

oalders commented Nov 30, 2016

@Varadinsky would you be able to rework your pull request to move the fix from the module to the tests as discussed above?

@Varadinsky

This comment has been minimized.

Show comment
Hide comment
@Varadinsky

Varadinsky Nov 30, 2016

Contributor

I also forgot to mention that I had the same issue on my dev box at work, and that this change fixed it. The width returned from _build_term_width() that caused the fail was 190.

If you still prefer the regexp approach let me know and I will rework the test.

Contributor

Varadinsky commented Nov 30, 2016

I also forgot to mention that I had the same issue on my dev box at work, and that this change fixed it. The width returned from _build_term_width() that caused the fail was 190.

If you still prefer the regexp approach let me know and I will rework the test.

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Nov 30, 2016

Owner

@Varadinsky I have no doubt that your changes fix the test, but they also pin the term_width at 80. I'd like the width to be sized automatically depending on how much space is available. Pinning it prevents that from happening.

We could look at pinning the width only in the tests, but that may prevent other bugs from being exposed.

The test makes an incorrect assumption, so I think fixing that is the way to go here. I appreciate you looking at this. Thanks for participating in the Pull Request Challenge. :)

Owner

oalders commented Nov 30, 2016

@Varadinsky I have no doubt that your changes fix the test, but they also pin the term_width at 80. I'd like the width to be sized automatically depending on how much space is available. Pinning it prevents that from happening.

We could look at pinning the width only in the tests, but that may prevent other bugs from being exposed.

The test makes an incorrect assumption, so I think fixing that is the way to go here. I appreciate you looking at this. Thanks for participating in the Pull Request Challenge. :)

@Varadinsky

This comment has been minimized.

Show comment
Hide comment
@Varadinsky

Varadinsky Dec 3, 2016

Contributor

:-) thanks for the quick feedback! Changes pushed.

Contributor

Varadinsky commented Dec 3, 2016

:-) thanks for the quick feedback! Changes pushed.

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders
Owner

oalders commented Dec 3, 2016

Thanks @Varadinsky!

@oalders oalders merged commit 36a4651 into oalders:master Dec 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

oalders added a commit that referenced this pull request Dec 4, 2016

v0.000033
    - Fix test that failed under some terminal widths (Varadinsky).
      #8

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Sep 27, 2017

wiz
p5-LWP-ConsoleLogger: update to 0.000036.
0.000036  2017-07-21 22:02:23Z
    - Re-log the request headers when we get the response
    - Don't skip Cookie and Set-Cookie when dumping headers

0.000035  2017-03-01 14:38:00-05:00 America/Toronto
    - Adds LWP::ConsoleLogger::Everywhere (simbabque)

0.000034  2017-01-23 09:24:06-05:00 America/Toronto
    - Don't redact x-www-form-urlencoded data.

0.000033  2016-12-03 21:46:12-05:00 America/Toronto
    - Fix test that failed under some terminal widths (Varadinsky).
      oalders/lwp-consolelogger#8

0.000032  2016-11-29 14:58:25-05:00 America/Toronto
    - Maybe guess at content type when parsing body.

0.000031  2016-11-25 13:08:50-05:00 America/Toronto
    - Display request body if available.

0.000030  2016-09-13 17:55:24-04:00 America/Toronto
    - Document header and parameter redaction
    - Suppress use of tables in some instances via "pretty"

0.000029  2016-09-13 10:37:57-04:00 America/Toronto
    - Stop setting default headers via debug_ua()

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Oct 22, 2017

wiz
p5-LWP-ConsoleLogger: update to 0.000036.
0.000036  2017-07-21 22:02:23Z
    - Re-log the request headers when we get the response
    - Don't skip Cookie and Set-Cookie when dumping headers

0.000035  2017-03-01 14:38:00-05:00 America/Toronto
    - Adds LWP::ConsoleLogger::Everywhere (simbabque)

0.000034  2017-01-23 09:24:06-05:00 America/Toronto
    - Don't redact x-www-form-urlencoded data.

0.000033  2016-12-03 21:46:12-05:00 America/Toronto
    - Fix test that failed under some terminal widths (Varadinsky).
      oalders/lwp-consolelogger#8

0.000032  2016-11-29 14:58:25-05:00 America/Toronto
    - Maybe guess at content type when parsing body.

0.000031  2016-11-25 13:08:50-05:00 America/Toronto
    - Display request body if available.

0.000030  2016-09-13 17:55:24-04:00 America/Toronto
    - Document header and parameter redaction
    - Suppress use of tables in some instances via "pretty"

0.000029  2016-09-13 10:37:57-04:00 America/Toronto
    - Stop setting default headers via debug_ua()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment