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.

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

Improve progress alignment #10940

Closed
brl0 opened this issue Apr 24, 2023 · 3 comments · Fixed by #10958
Closed

Improve progress alignment #10940

brl0 opened this issue Apr 24, 2023 · 3 comments · Fixed by #10958
Labels
topic: reporting related to terminal output and user-facing messages and errors type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@brl0
Copy link
Contributor

brl0 commented Apr 24, 2023

First of all, thanks to all contributors, your work is much appreciated!

The right aligned percent progress looks great in the terminal, but sometimes the nice alignment is marred by verbose (-vv+) output.
Here is an example of the current alignment:

tests/implementations/test_hdfs.py::TestUPathHDFS::test_fsspec_compat SKIPPED (Possible installation
 problem, error: Unable to load libhdfs: ./libhdfs.so: cannot open shared object file: No such file 
or directory) [ 49%]
tests/test_core.py::TestUPathMock::test_instance_type <- upath/tests/cases.py PASSED          [ 50%]

I was thinking that it would be nice to keep the percentages aligned even for multiline output. It struck me that this could be done by always padding to the rounded up next multiple of terminal width. I think this would make the output in these situations look much nicer.

Here is an example of how the output could be improved:

tests/implementations/test_hdfs.py::TestUPathHDFS::test_fsspec_compat SKIPPED (Possible
installation problem, error: Unable to load libhdfs: ./libhdfs.so: cannot open shared object
file: No such file or directory)                                                              [ 49%]
tests/test_core.py::TestUPathMock::test_instance_type <- upath/tests/cases.py PASSED          [ 50%]

I wrote a function using python's built-in text wrap module that should be easy to drop in to accomplish this, along with a test.

import textwrap

_DEFAULT_PROGRESS_MARGIN_SIZE = 7

def _pad_wrap_message(
    msg: str,
    width: int,
    margin: int = _DEFAULT_PROGRESS_MARGIN_SIZE,
    line_sep: str="",
):
    """Wrap and pad message with margin for progress info."""
    wrapped = textwrap.wrap(msg, width - margin, drop_whitespace=True)
    out_lines = [line.ljust(width) for line in wrapped[:-1]]
    out_lines.append(wrapped[-1].ljust(width - margin))
    msg_out = line_sep.join(out_lines)
    return msg_out

def test_pad_wrap_message():
    width = 80
    count = 200
    msg = "b"
    progress = " [100%]"
    margin = len(progress)
    result = _pad_wrap_message(msg * count, width=width, line_sep="\n")
    assert result.count(msg) == count
    lines = (result + progress).split("\n")
    expected_lines = (len(msg) * count) // (width - margin) + 1
    assert len(lines) == expected_lines
    expected_len = expected_lines * (width + 1) - margin - 1
    assert len(result) == expected_len
    for _ in lines:
        assert len(_) == width
        assert _[-len(progress)] == " "  # space before progress column

If this approach is acceptable, I can submit a PR.

@nicoddemus
Copy link
Member

Hi @brl0,

Thanks for writing!

I would be glad to review/merge a PR in that regard, for sure.

@nicoddemus nicoddemus added type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature topic: reporting related to terminal output and user-facing messages and errors labels Apr 26, 2023
@brl0
Copy link
Contributor Author

brl0 commented May 1, 2023

I submitted PR #10958, which is simpler than what I was originally proposing.

In that PR, textwrap is only applied to the formatted reason for skip/xfail. I considered applying to the test path/name, but wasn't sure if that would be accepted, so I thought I would check-in on that first. The main drawback I can think of is that it might interfere with multiline copy of test path/name by adding some spacing before the line-break.

@nicoddemus, what are your thoughts? If that change is desirable, I will probably do so in a separate PR.

@nicoddemus
Copy link
Member

I agree it is probably best to not touch the node id at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: reporting related to terminal output and user-facing messages and errors type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants