Add helper for formatting long warning messages#2563
Add helper for formatting long warning messages#2563otekraden wants to merge 20 commits intobeeware:mainfrom
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).
src/briefcase/utils.py
Outdated
| 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.
src/briefcase/utils.py
Outdated
| 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.
src/briefcase/console.py
Outdated
| :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. |
src/briefcase/console.py
Outdated
| 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, |
src/briefcase/console.py
Outdated
| # 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.
src/briefcase/utils.py
Outdated
| 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.
| experience in open source | ||
| development. | ||
| I like your project, and I'm excited to contribute to it.\ | ||
| """ |
There was a problem hiding this comment.
Is the use of an f-string testing anything specific here? I'm fairly sure this can be simplified to a static string definition.
| 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) |
src/briefcase/console.py
Outdated
| 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.
src/briefcase/console.py
Outdated
| 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.
src/briefcase/console.py
Outdated
| 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?
src/briefcase/console.py
Outdated
| 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?
|
Good afternoon, I've completed all the requested code edits. While implementing the changes, I encountered an issue with "tox": I couldn't reach 100% test coverage. I discovered that running the "tox p" command after making edits allows the coverage to be recalculated correctly. I tried to use created feature in the Android SDK integrations, but tests falling in errors(( |
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 |

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: