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.

Sign up for GitHub

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

Jump to bottom

Fix table layout #335

Merged
3 commits merged into from May 31, 2023
Merged

Fix table layout #335

3 commits merged into from May 31, 2023

Conversation

ghost
Copy link

@ghost ghost commented May 30, 2023

Purpose

PR #307 updates how the table layout is computed. It appears that the severity strings are translated and this can break the layout. This PR fixes that by computing the width taking into account the length of the translated severity strings.

Context

Follow-up #307.
Fixes #334

Changes

  • update severity column width computation

How to test this PR

Check that when changing the level and the locale, the table layout does not break.

zonemaster-cli --locale es_ES.utf8 --level CRITICAL zonemaster.net
zonemaster-cli --locale es_ES.utf8 --level DEBUG zonemaster.net
zonemaster-cli --locale sv_SE.utf8 --level CRITICAL zonemaster.net

@ghost ghost added the T-Bug Type: Bug in software or error in test case description label May 30, 2023
@ghost ghost requested a review from mattias-p May 30, 2023 12:41
mattias-p
mattias-p previously approved these changes May 30, 2023
View reviewed changes
@matsduf matsduf added this to the v2023.1 milestone May 30, 2023
@matsduf
Copy link
Contributor

matsduf commented May 30, 2023

I assumed that the milestone for this PR should be v2023.1.

@matsduf matsduf added the V-Patch Versioning: The change gives an update of patch in version. label May 30, 2023
@tgreenx tgreenx linked an issue May 30, 2023 that may be closed by this pull request
Long severity level translations break table layout in output #334
Closed
@ghost ghost requested a review from matsduf May 30, 2023 13:25
matsduf
matsduf requested changes May 30, 2023
View reviewed changes
Copy link
Contributor

@matsduf matsduf left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really how it should look like?

# zonemaster-cli --locale es_ES.UTF-8 lame.dufberg.se

Segundos Nivel     Mensaje
======== ========= =======
   23.29 CRÍTICO  There is no working name server for "lame.dufberg.se" so it is unreachable.
   23.29 ADVERTENCIA Name server "ns1.lame.dufberg.se/159.253.30.207" does not respond to an SOA query.
   23.29 ADVERTENCIA Name server "ns2.lame.dufberg.se/188.126.83.221" does not respond to an SOA query.
   23.29 CRÍTICO  No hay suficiente información sobre lame.dufberg.se que permita ejecutar las pruebas.

@ghost
Copy link
Author

ghost commented May 30, 2023 •
edited by ghost
Loading

Is this really how it should look like?

Not exactly.

The output should be (manually edited):

Segundos Nivel       Mensaje
======== =========== =======
   23.29 CRÍTICO     There is no working name server for "lame.dufberg.se" so it is unreachable.
   23.29 ADVERTENCIA Name server "ns1.lame.dufberg.se/159.253.30.207" does not respond to an SOA query.
   23.29 ADVERTENCIA Name server "ns2.lame.dufberg.se/188.126.83.221" does not respond to an SOA query.
   23.29 CRÍTICO     No hay suficiente información sobre lame.dufberg.se que permita ejecutar las pruebas.

With the current code, the output I get is: (no longer true with the latest updates to this PR)

Segundos Nivel       Mensaje
======== =========== =======
   23.25 CRÍTICO    There is no working name server for "lame.dufberg.se" so it is unreachable.
   23.25 ADVERTENCIA Name server "ns2.lame.dufberg.se/188.126.83.221" does not respond to an SOA query.
   23.25 ADVERTENCIA Name server "ns1.lame.dufberg.se/159.253.30.207" does not respond to an SOA query.
   23.25 CRÍTICO    No hay suficiente información sobre lame.dufberg.se que permita ejecutar las pruebas.

It seems the Í and its accent are not properly handled. I haven't found a proper solution yet. I'll look into binmode() and see what I can get. Any other ideas are welcome.

Alexandre Pion added 2 commits May 30, 2023 17:09
Refactoring
ab25b3f
Replace sprintf usage
66ec177 
Perl sprintf does not handle unicode. To avoid misalignment when dealing
with translated severity strings, the previous sprintf call is replaced
with a manual concatenation of the string and the remaining spaces.
@ghost
Copy link
Author

ghost commented May 30, 2023

It seems the Í and its accent are not properly handled. I haven't found a proper solution yet. I'll look into binmode() and see what I can get. Any other ideas are welcome.

I found binmode too hard to use and understand. I chose to replace the sprintf usage with a manual concatenation of the translated string and the remaining spaces. Now you should get a proper output. Please re-review.

@matsduf
Copy link
Contributor

matsduf commented May 30, 2023

My objection above was based on an error on my side. The issue that @PNAX found with Í was real. I guess that it was because it is a non-ASCII character and incorrectly counted as several character because of its UTF-8 representation.

At least that works as expected now:

Segundos Nivel       Mensaje
======== =========== =======
   23.18 CRÍTICO     There is no working name server for "lame.dufberg.se" so it is unreachable.
   23.18 ADVERTENCIA Name server "ns1.lame.dufberg.se/159.253.30.207" does not respond to an SOA query.
   23.18 ADVERTENCIA Name server "ns2.lame.dufberg.se/188.126.83.221" does not respond to an SOA query.
   23.18 CRÍTICO     No hay suficiente información sobre lame.dufberg.se que permita ejecutar las pruebas.

This is a nice fix.

@ghost ghost merged commit 773a0c5 into zonemaster:develop May 31, 2023
@ghost ghost deleted the fix-layout branch May 31, 2023 09:15
@matsduf matsduf mentioned this pull request Jun 7, 2023
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
T-Bug Type: Bug in software or error in test case description V-Patch Versioning: The change gives an update of patch in version.
Projects
None yet
Milestone
v2023.1
Development

Successfully merging this pull request may close these issues.

Long severity level translations break table layout in output
2 participants
@matsduf @mattias-p