Add helper for formatting long warning messages#2563
Conversation
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The broad strokes of this look like it's on the right track - but there's a few details to sort out. Some of them I've flagged inline; but there are a couple of bigger picture items.
Firstly, this is currently failing CI because of a town crier check; see our contribution guide for details on what that means.
Secondly - we try to avoid generic "utils" modules. In this case, the code is a very specific addition to console handling - so it should be part of the console module. Given that this will always be used in the context of a console.warning(), it would make sense to me for this to be a standalone method on Console - something like console.warning_banner(title, body, width=80). It may help to keep the "formatting" part of the code separate so that console._banner() returns a string, which is then output by console.warning_banner() (and provides an opportunity for error_banner() if we ever need it).
| Defaults to 2. | ||
|
|
||
| Returns: | ||
| str: The formatted message. |
There was a problem hiding this comment.
We use the Sphinx docstring format; check any other source file for examples of use.
There was a problem hiding this comment.
Got it!
I applied this format.
| lines.append(border_line) | ||
|
|
||
| # split the message into paragraphs | ||
| paragraphs = message.split("\n") |
There was a problem hiding this comment.
We should probably use textwrap.dedent() here as a convenience as well.
There was a problem hiding this comment.
Sorry, but in my current understanding of the logic of my function, I don't see how to use the textwrap.dedent() method. Could you please show me how to integrate it?
There was a problem hiding this comment.
What I'm suggesting is that being able to call:
console.warning_banner(
"Some banner title"
"""\
This is the content that I want to display. It is long
but I don't want to have to manually word wrap\n
\n
But manual line breaks should be preserved.
"""
)
We could do this by requiring the user to wrap the """ string in textwrap.dedent()... or we could do that automatically on every usage.
|
@freakboy3742 |
…sole primitives section. Tests relocated to Console tests folder.
|
I have attempted to make all the necessary changes based on the instructions above. |
|
I rewrote Console method from static to instance method. |
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for those cleanups. The CI failure you saw was unrelated - we had some disk space issues in our CI configuration. We've fixed those now, and I've re-run the affected tests, and they're passing now.
I've left a couple more review comments; the only other suggestion is that it could use a couple more tests.
In particular, this is a situation where parameterized tests would be helpful. There's a general pattern of a set of input. Then a need to test how that input renders at different widths - widths that are longer than the text, the same width as the text, less than the width of the text, and so on.
Lastly, it would be useful to actually use this API somewhere. It doesn't need to update every usage of banner-like output, but if you pick a module (like the Android integrations), and replace every warning banner with the equivalent usage of this utility, we can see that it will work in practice.
| :param message: The message to format inside the box. | ||
| :type message: str | ||
| :param title: The title of the box. If provided, appears centered at the top. | ||
| :type title: str, optional | ||
| :param width: The total width of the box in characters. Defaults to 80. | ||
| :type width: int, optional | ||
| :param border_char: Character to use for the box border. Defaults to "*". | ||
| :type border_char: str, optional | ||
| :param padding: Number of spaces between the border and text content. Defaults | ||
| to 2. | ||
| :type padding: int, optional | ||
| :return: The formatted message enclosed in a bordered box. | ||
| :rtype: str |
There was a problem hiding this comment.
Apologies for the miscommunication here; but we don't need the type documentation - because they are (or should be) in the type annotations for the method itself.
| :param message: The message to format inside the box. | |
| :type message: str | |
| :param title: The title of the box. If provided, appears centered at the top. | |
| :type title: str, optional | |
| :param width: The total width of the box in characters. Defaults to 80. | |
| :type width: int, optional | |
| :param border_char: Character to use for the box border. Defaults to "*". | |
| :type border_char: str, optional | |
| :param padding: Number of spaces between the border and text content. Defaults | |
| to 2. | |
| :type padding: int, optional | |
| :return: The formatted message enclosed in a bordered box. | |
| :rtype: str | |
| :param message: The message to format inside the box. | |
| :param title: The title of the box. If provided, appears centered at the top. | |
| :param width: The total width of the box in characters. Defaults to 80. | |
| :param border_char: Character to use for the box border. Defaults to "*". | |
| :param padding: Number of spaces between the border and text content. Defaults | |
| to 2. | |
| :return: The formatted message enclosed in a bordered box. |
| width=80, | ||
| border_char="*", | ||
| padding=2, |
There was a problem hiding this comment.
Missing type annotations:
| width=80, | |
| border_char="*", | |
| padding=2, | |
| width: int = 80, | |
| border_char: str = "*", | |
| padding: int = 2, |
| # Wrap the title to lines to fit the width of the box | ||
| wrapped_title_lines = textwrap.wrap( | ||
| title.strip(), | ||
| width=width - (padding * 2) - 4, |
There was a problem hiding this comment.
I'm not sure there's a use case for configurable padding here - 1 space of padding in the title will always be the default.
| lines.append(border_line) | ||
|
|
||
| # split the message into paragraphs | ||
| paragraphs = message.split("\n") |
There was a problem hiding this comment.
What I'm suggesting is that being able to call:
console.warning_banner(
"Some banner title"
"""\
This is the content that I want to display. It is long
but I don't want to have to manually word wrap\n
\n
But manual line breaks should be preserved.
"""
)
We could do this by requiring the user to wrap the """ string in textwrap.dedent()... or we could do that automatically on every usage.
Padding arg removed: it will be always 1.
…, returns empty string.
|
I have made the following changes:
|
|
@otekraden Hello! Thank you for submitting the requested changes. As you can see, the CI is failing. We maintain 100% code coverage in testing. The current CI issue appears to be related to the PR now being less-than 100% coverage. You can click on the failure to see exactly where the issue is. Please get this PR passing before we review it again. If you have questions or need help, please post here. Thanks! |
|
@kattni @freakboy3742
I will be grateful for any hint. |
freakboy3742
left a comment
There was a problem hiding this comment.
This is looking good! A few review comments inline.
Regarding the coverage failure - the error is telling you that a specific line of code isn't being exercised by the test suite. The fix is to add a test case that will exercise that line of code - in this case, the case of a message that isn't a string.
That said - as noted by my review comments, that line of code probably isn't needed, because we can assume the method is being used by a "reasonable person" who is working with Briefcase. It doesn't necessarily need to be robust enough to handle any random input from an end user.
The last suggestion - at present, this PR adds a new feature, and does a good job testing; however, it would be good to see a few examples of the API being used in practice. You don't need to update every banner in the codebase - but if you pick one module (say, the Android SDK integrations in src/briefcase/integrations/android_sdk.py) and replace all banners with usage of this API, you can prove that the API is "fit for purpose".
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("test_name", "message", "title", "width", "expected"), |
There was a problem hiding this comment.
There's no need to include a descriptive test name - your test doesn't use it anyway:
| ("test_name", "message", "title", "width", "expected"), | |
| ("message", "title", "width", "expected"), |
If you do want to provide a test identifier, use pytest.param() to define each group of test data; the ID in that case should be something short (like "test-1" or "empty-message-title") that can be used to run a specific test. This isn't required, but it can be a nice convenience.
| 80, | ||
| """\ | ||
| ******************************************************************************** | ||
| ** ** |
There was a problem hiding this comment.
This extra blank link in the title is a little odd. It wouldn't match any existing uses of warning banners.
| # # Debug output if needed | ||
| # print('\n\n' + test_name) | ||
| # print("Generated:") | ||
| # print(msg) | ||
| # print("Expected:") | ||
| # print(expected) | ||
|
|
There was a problem hiding this comment.
There's no need to retain this debug code.
| # # Debug output if needed | |
| # print('\n\n' + test_name) | |
| # print("Generated:") | |
| # print(msg) | |
| # print("Expected:") | |
| # print(expected) |
| if not any((message, title)) or not isinstance(width, int): | ||
| return "" | ||
|
|
||
| def dedent_and_split(text): |
There was a problem hiding this comment.
Rather than defining this as an inline function, it should be a private method on the base Console class (i.e. , _dedent_and_split().
I'd also suggest that we can add an extra affordance around the handling of input strings. As written, I think this requires that the input string has a trailing \ on the first line (i.e., warning_banner(f"""\. That's the sort of detail that will be consistently forgotten in practice. Since the effect of the \ is to suppress the newline on the first character, it might be worth adding a text.strip('\n') so that any leading or trailing newlines are removed from the input string, ensuring that we're in complete control of the spacing around the message in the box.
| if not any((message, title)) or not isinstance(width, int): | ||
| return "" |
There was a problem hiding this comment.
Wouldn't it make more sense to raise an error if the arguments aren't the right type?
It's also worth asking if any error handling needed at all. This isn't a public API - it's an internal utility; we don't have to be rock solid in the API we expose, because we're in control of how the method is used.
| if not isinstance(title, str): | ||
| return "" |
There was a problem hiding this comment.
If we're doing error handling, we should be raising an error, not failing silently.
But again - is any error handling needed?
| if not isinstance(message, str): | ||
| return "" |
There was a problem hiding this comment.
As above - is this error handling needed; and if it is, shouldn't it be error handling?
No problem at all - we're grateful for any time you can volunteer.
I'm not sure I understand what you're describing here - you'll need to provide some more details here. The current test suite isn't reporting any coverage problems.
This is to be expected. You've made changes that alter the output of the Android SDK by switching to the new banner API; the tests that are failing are reporting that the output of some commands has changed. Once you've confirmed that the changes are expected, you'll need to update the tests to reflect the new output. |
dcdb43c to
f6def9c
Compare
…removed from args.
…; test mod according this change.
|
@freakboy3742 |
freakboy3742
left a comment
There was a problem hiding this comment.
Ok - so this is why we wanted to see some examples of usage - there's clearly something not right here. See my comment inline about what this should look like from the perspective of the user.
| tools.console.warning_banner( | ||
| title=""" | ||
| WARNING: ANDROID_HOME and ANDROID_SDK_ROOT are inconsistent | ||
| """, | ||
| message=f""" | ||
| The ANDROID_HOME and ANDROID_SDK_ROOT environment variables are set | ||
| to different paths: | ||
|
|
||
| ANDROID_HOME: {android_home} | ||
| ANDROID_SDK_ROOT: {android_sdk_root} | ||
|
|
||
| to different paths:\\n\\n | ||
| ANDROID_HOME: {android_home}\\n | ||
| ANDROID_SDK_ROOT: {android_sdk_root}\\n\\n | ||
| Briefcase will ignore ANDROID_SDK_ROOT and only use the path | ||
| specified by ANDROID_HOME. | ||
| specified by ANDROID_HOME.\\n\\n | ||
|
|
||
| You should update your environment configuration to either not set | ||
| ANDROID_SDK_ROOT, or set both environment variables to the same | ||
| path. | ||
|
|
||
| ************************************************************************* | ||
| """ | ||
| """, |
There was a problem hiding this comment.
The title being expressed as a triple string over three lines is... very odd; and the need for large numbers of \\n characters in the input for dialogs clearly indicates that the implementation isn't right. There may be places where a manual \n is required, but they should be the extreme exception, not the rule.
At the very least, a blank line in the input should be interpreted as a blank line in the output, without any need for special handling. My immediate impression is that the simple "replace '\n' with ' '" isn't right; we should only be replacing a newlines where the next character isn't a newline or whitespace. That would preserve both the "layout" blank lines that are common at present, as well as the ANDROID_HOME indented example here.
There was a problem hiding this comment.
Also - if the indentation isn't being preserved in output, then we can simplify and remove it from the input as well. Part of the reason of introducing this wizard is so that we can put the message "in context", rather than needing to have it in code the way we want it to be displayed.
… output; relative indentation kept tests modified
|
@freakboy3742 Hello! I am making another attempt to complete work on this task. |
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for those updates; however, based on the changes you've made here, I think we've still got a miscommunication here about the purpose of this feature.
The reason that we want to introduce a general purpose banner that allows us to put error messages into code without needing to have them "left aligned" in the code.
A few months ago, I gave you the following example of usage:
console.warning_banner(
"Some banner title"
"""\
This is the content that I want to display. It is long
but I don't want to have to manually word wrap\n
\n
But manual line breaks should be preserved.
"""
)
(It's probably arguable whether the \n chars in that example are needed - in that context, the newlines aren't ambiguous)
This is an example of the sort of usage we want to see - a banner that will be displayed to the user with 80 character line widths, but doesn't need to be rendered in the code with 80 character line widths.
In this PR, in your sample usage, you haven't modified any of the actual content for the Android SDK messages. That turns your warning_banner() method into a very large and complex wrapper for putting '*'*80 around the top and bottom of a message. The rest of the logic for de-indenting and reflowing content essentially isn't being used at all.
What I'm expecting to see here is examples of usage that actually make use of the new capability that has been added here. I've updated one example to show the sort of change that I'm talking about; the same sort of update should be applied to every message.
I'm expecting that this will likely reveal other possible bugs and usage issues - which is why I've asked for a practical example of usage. It's not enough to just build an API - we need to demonstrate that it will actually be useful in practice for the purpose it is intended.
One more detail: this PR is currently raising an error when documentation is built; this is because the docs contain a stale URL reference. This has been fixed in main; if you merge the main branch into this code, that problem will be resolved.
| tools.console.warning_banner( | ||
| "WARNING: Using Android SDK from ANDROID_SDK_ROOT", |
There was a problem hiding this comment.
This usage should be something like:
| tools.console.warning_banner( | |
| "WARNING: Using Android SDK from ANDROID_SDK_ROOT", | |
| tools.console.warning_banner( | |
| title="Using Android SDK from ANDROID_SDK_ROOT", | |
| message=f"""\ | |
| Briefcase is using the Android SDK specified by the | |
| ANDROID_SDK_ROOT environment variable. | |
| Android has deprecated ANDROID_SDK_ROOT in favor of | |
| the ANDROID_HOME environment variable. | |
| Update your environment configuration to set | |
| ANDROID_HOME instead of ANDROID_SDK_ROOT to ensure | |
| future compatibility. | |
| """ | |
| ) |
Note that if we're including "WARNING: " in every message, we can hard code that text.
Added indentation for all warning messages to follow code structure. Removed "WARNING: " prefixes from titles.
|
I've fixed the method calls in "android_sdk". In addition, I encoded the "WARNING:" prefix of the headers. I really hope that now my solution looks like what you require. |
freakboy3742
left a comment
There was a problem hiding this comment.
Ok - the text is now indented correctly in the source - but did you actually try it?
I did the following:
export ANDROID_HOME=something
export ANDROID_SDK_ROOT=somethingelse
briefcase create android
The values of the two environment variables don't matter - just that they are different, and not a valid SDK. If you do this, you get the following:
So... the text in the warning banners isn't being reflowed to 80 characters. The tests cases show that it can reflow... but in practice, it isn't. This is why I asked for examples of usage.
…c of splitting input text to paragraphs. Description of "console.warning_banner" was changed. Tests in "test_warning_banner" modified.
| To separate text into paragraphs you can use: | ||
| - blank line; | ||
| - relative indentation. | ||
| Lines with zero relative indentation will be merged to upper paragraph. |
There was a problem hiding this comment.
I rewrote the logic of splitting the text into paragraphs.
The width of each paragraph is equal to the specified border width minus the relative indentation.
|
@freakboy3742 The result of the method is now as follows: Paragraphs take up the full width of the box minus relative indentation. |
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for those updates. I've pushed some updates to simplify the implementation and remove some safety checks that aren't needed, and slightly tweak the presentation for some content (e.g, adding padding around the message content, and preserving indented lines, as they're an indicator of shell literal content).
This will be a big improvement for future developers - thanks!
| # If message and title are both empty rase ValueError | ||
| if not any((message, title)): | ||
| raise ValueError("Message or title must be provided") | ||
| # If width is not an integer, raise TypeError | ||
| if not isinstance(width, int): | ||
| raise TypeError("Width must be an integer") |
There was a problem hiding this comment.
There's no need to do this sort of error checking. It's an internal tool; we can rely on developers to use it correctly.
| """ | ||
|
|
||
| # character to use for the box border | ||
| border_char = "*" |
There was a problem hiding this comment.
Unless we're going to need to change this, it's a lot clearer to just use * where it's needed.
| if not isinstance(title, str): | ||
| raise TypeError("Title must be a string") |
There was a problem hiding this comment.
Again - we can rely on developers to use this right.
| # create lines array with opening line of the box | ||
| lines_array = [border_line] | ||
|
|
||
| # if title exists, format title in the box |
There was a problem hiding this comment.
Be wary of using excessive comments - they end up being more confusing than helpful.
| # create paragraphs from lines | ||
| paragraphs = [] |
There was a problem hiding this comment.
This is an area where comments would have been helpful - because "paragraphs" isn't a list of paragraphs - it's a list of lists, where each element is a line, and the "rel_indent" value.
However, this entire logic can be a lot simpler.
| # Add a warning prefix for title | ||
| if is_title: | ||
| text = f"WARNING: {text.strip()}" |
There was a problem hiding this comment.
This is the wrong place to add WARNING - add it as part of the input not the wrapping mechanism.
| @@ -0,0 +1 @@ | |||
| ``Warning_banner`` method added to ``Console`` class. | |||
There was a problem hiding this comment.
This is markdown, not RST (i.e., use single quotes).
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("message", "title", "width", "expected"), |
There was a problem hiding this comment.
Given the function takes "title" then "message", it makes sense to preserve that ordering in tests.
| ******************************************************************************** | ||
| """, | ||
| ), | ||
| # Custom width (60) with title and message |
There was a problem hiding this comment.
This test case isn't really testing anything new - it's "just another width". Test cases should always be exercising something new.


I solved Issue #2559 by creation the function "format_message_to_asterisk_box", that formatting message to asterisk box representation.
It takes this args:
message (str): The message to format.
title (str, optional): The title of the box. Defaults to None.
width (int, optional): The width of the box. Defaults to 80.
border_char (str, optional): The character to use for the border.
Defaults to "*".
padding (int, optional): The number of spaces to pad the border.
Defaults to 2.
Also I added tests for it. Function it seems working good.
But I was not sure in what file to put function.
Now its location and tests by this paths:
src/briefcase/utils.py
tests/test_utils.py
It is my very first attempt of contribution. Please don beat me hard)
I am waiting for your feedback.
PR Checklist: