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

Use ljust to left justify and pad strings #5254

Conversation

jrafanie
Copy link
Member

Text based reports ignore line widths. Performing manual padding was
causing a "negative argument" error if a line exceeded the normal
report's line width.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1677839

@jrafanie
Copy link
Member Author

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Feb 19, 2019
@jrafanie jrafanie force-pushed the fix_padding_calculation_since_text_ignores_line_width branch from 4fdb218 to aa7ee41 Compare February 19, 2019 22:20
@jrafanie
Copy link
Member Author

cc @gtanzillo

@mzazrivec
Copy link
Contributor

Current CI failures seem related:

ReportFormatter::ReportText expands report width for really long filter condition
     Failure/Error: expect(line.strip).to eq(expected.lines[index].strip)
     
       expected: "|( Performance - VM : Activity Sample - Timestamp (Day/Time) IS \"Last Hour\" AND Performance - VM : CPU - Usage Rate for Collec>>"
            got: "|( Performance - VM : Activity Sample - Timestamp (Day/Time) IS \"Last Hour\" AN>>"
     
       (compared using ==)
     # ./spec/lib/report_formater/text_formatter_spec.rb:29:in `block (4 levels) in <top (required)>'
     # ./spec/lib/report_formater/text_formatter_spec.rb:28:in `each'
     # ./spec/lib/report_formater/text_formatter_spec.rb:28:in `each_with_index'
     # ./spec/lib/report_formater/text_formatter_spec.rb:28:in `block (3 levels) in <top (required)>'
     # ./spec/lib/report_formater/text_formatter_spec.rb:11:in `block (2 levels) in <top (required)>'

@jrafanie
Copy link
Member Author

yeah, @mzazrivec, interesting... they pass locally... I wonder if another test is modifying class ivars. I'll fix it up and let you know when it's ready to go.

Text based reports ignore line widths.  Performing manual padding was
causing a "negative argument" error if a line exceeded the normal
report's line width.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1677839
@jrafanie jrafanie force-pushed the fix_padding_calculation_since_text_ignores_line_width branch from aa7ee41 to 10f082f Compare February 20, 2019 20:50
@miq-bot
Copy link
Member

miq-bot commented Feb 20, 2019

Checked commit jrafanie@10f082f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

@jrafanie
Copy link
Member Author

ok @mzazrivec, it's ready to go. I had to change the test to ensure it was testing the scenario in which it was not truncating long lines. Thanks!

@mzazrivec mzazrivec self-assigned this Feb 21, 2019
@mzazrivec mzazrivec added this to the Sprint 106 Ending Mar 4, 2019 milestone Feb 21, 2019
@mzazrivec
Copy link
Contributor

@jrafanie changes look good, thanks.

Also, I hope you did not get too puzzled by Чук and Гек.

@mzazrivec mzazrivec merged commit 805c194 into ManageIQ:master Feb 21, 2019
simaishi pushed a commit that referenced this pull request Mar 29, 2019
…text_ignores_line_width

Use ljust to left justify and pad strings

(cherry picked from commit 805c194)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1693727
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit 6fce3b0530688344df75cb5d460222956f07bdff
Author: Milan Zázrivec <[email protected]>
Date:   Thu Feb 21 08:11:36 2019 +0100

    Merge pull request #5254 from jrafanie/fix_padding_calculation_since_text_ignores_line_width
    
    Use ljust to left justify and pad strings
    
    (cherry picked from commit 805c1944ce9bdfbbee6bb3f54a179d036908e631)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1693727

@jrafanie jrafanie deleted the fix_padding_calculation_since_text_ignores_line_width branch October 29, 2019 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants