-
Notifications
You must be signed in to change notification settings - Fork 904
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
doctor: improve error messages #746
Conversation
… `androidHomeEnvVariable`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing and we're good
command, | ||
)}`, | ||
); | ||
addBlankLine(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the one that causes extra space which is not necessary imho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which space do you mean? the one before iOS
or the one after iOS
is done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's because there's a space after each health check category (in this case Common
and iOS
) and also a space after each error is shown, that's why that extra space is there.
To be sincere I actually tried to get rid of it but there isn't an easy way to do at the moment, if we remove this last space then it can get to a funky UI when the previous health check succeeded but the next one failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best way to do this is to actually check if the last health check failed, if so then print only one space instead of two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's figure this out later, this is a very minor inconvenience
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for commenting about this though!
Summary:
This is related to #694, it improves the error messages by maintaining consistency between the health checks with one separated function to log errors.
It also fixes a problem introduced on #743 where it shows
undefined
when thedescription
is not provided on a health check.Before
After
Test Plan:
/path/to/cli doctor
.