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

[WFCORE-3540] hardcode terminal width and height to avoid malformed t… #3062

Merged
merged 1 commit into from Feb 23, 2018
Merged

[WFCORE-3540] hardcode terminal width and height to avoid malformed t… #3062

merged 1 commit into from Feb 23, 2018

Conversation

soul2zimate
Copy link
Contributor

…able view in non interactive mode.
https://issues.jboss.org/browse/WFCORE-3540

jfdenise
jfdenise reviewed Jan 24, 2018
View reviewed changes
cli/src/main/java/org/jboss/as/cli/impl/CommandContextImpl.java Outdated
@@ -1650,7 +1650,7 @@ public int getTerminalWidth() {
@Override
public int getTerminalHeight() {
if( !INTERACT ){
return 0;
return 40;
Copy link
Contributor

Choose a reason for hiding this comment

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

Strangely, in the same method when an error occurs, 24 is returned. This value seems random. That is strange to have 24 returned in one case and 40 in another. I looked at GIT history but nothing special. You should perhaps try to replace 24 by 40, I can't see how this could have an effect.

@soul2zimate
Copy link
Contributor Author

will do.
I saw also two failures from AttachmentTestCase related to this change. I am looking into it.

@soul2zimate
Copy link
Contributor Author

Last run, AttachmentTestCase failed due to MissingFormatWidthException from Formatter. Because SimpleTable constructor allows to create object without headers, that makes an empty columnLengths array. It was previously filled with value.length() + 1 as it allows to have terminal width 0 in tests.
Once the hardcoded values replace zero, it bypass that step and column width remains 0.

@soul2zimate
Copy link
Contributor Author

@jfdenise how do you feel about the change in SimpleTable.java ?

jfdenise
jfdenise suggested changes Feb 19, 2018
View reviewed changes
Copy link
Contributor

@jfdenise jfdenise left a comment

Choose a reason for hiding this comment

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

Other than the tiny comment, that is good to go. Thank-you.

cli/src/main/java/org/jboss/as/cli/impl/CommandContextImpl.java Outdated
@@ -1650,7 +1650,7 @@ public int getTerminalWidth() {
@Override
public int getTerminalHeight() {
if( !INTERACT ){
return 0;
return 24;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add a comment saying that 24 has no special meaning except that it is a value greater than 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, comment is added.

@soul2zimate
Copy link
Contributor Author

test failure is unrelated.

jfdenise
jfdenise approved these changes Feb 22, 2018
View reviewed changes
Copy link
Contributor

@jfdenise jfdenise left a comment

Choose a reason for hiding this comment

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

Thanks, approved.

@bstansberry bstansberry added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Feb 23, 2018
@bstansberry bstansberry merged commit f5844b5 into wildfly:master Feb 23, 2018
@soul2zimate soul2zimate deleted the WFCORE-3540 branch February 26, 2018 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfdenise jfdenise jfdenise approved these changes

Assignees
No one assigned
Labels
ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
Milestone
No milestone
3 participants
@soul2zimate @jfdenise @bstansberry