Initial version of auto-detection of terminal width.

[dawngerpony]

Might fix #90.

• Use shutil.get_terminal_size to detect terminal size in Python 3
• Use backports.shutil_get_terminal_size to detect terminal size in Python 2, as per this SO answer. This appears to be more effective than the suggested approach of checking os.env['COLUMNS'] but does come at the price of a new dependency. The various answers in the SO link make an interesting read.
• Subtract the length of the prefix from the line wrap width to allow for the length of the prefix
• Make sure that this value is being passed through to the argToStringFunction, even if a custom function is being used instead of the default pprint, but only if the custom function supports a width parameter.
• Add a couple of tests
• Ensure that lineWrapWidth is being reset before the start of every test, for idempotency.

Notes

• The implementation of pprint in Python 2 doesn't wrap long strings so we test slightly different behaviour between Python 2 and Python 3.
• If we are not in a terminal, we get an OSError and abandon the attempt at detecting line width
• This has been verified on Mac OS 11.6.1 (Intel) in Python 2.7.18 and 3.9.7
• I don't have any other versions of Python installed so I didn't run tox against any other environments or in any other OSes.

[dawngerpony]

Update: it seems that the behaviour of pprint is different in Python 2.7 compared to 3.9. I can't make pprint wrap a long string across multiple lines in 2.7, so it doesn't appear to be respecting the width parameter. As such, I can't make the test that I have written pass in both 2.7 and 3.x. Any thoughts?

[alexmojaki]

Thanks for contributing!

IMO it's not worth spending significant time on perfecting 2.7 since it's well past EOL. I haven't looked carefully but it sounds like you've made a significant improvement for 3.x and not made anything worse for 2.7 and that's great. You can write something like expected = '...' if sys.version[0] == 2 else '...' or just @unittest.skipIf(sys.version[0] == 2) on the whole test function. Follow your heart.

[dawngerpony]

Thanks for contributing!

You're very welcome! I love your icecream package and am happy to give back.

IMO it's not worth spending significant time on perfecting 2.7 since it's well past EOL. I haven't looked carefully but it sounds like you've made a significant improvement for 3.x and not made anything worse for 2.7 and that's great. You can write something like expected = '...' if sys.version[0] == 2 else '...' or just @unittest.skipIf(sys.version[0] == 2) on the whole test function. Follow your heart.

That's awesome thanks. Unfortunately I had to skip the entire test because there's an assertion inside parseOutputIntoPairs() which is tripping up, and I can't skip that assertion just for one test.

But the test is committed, I'll mark this PR as ready.

[dawngerpony]

Any thoughts about this overflow thingy? I just realised that the extra characters might come from the ic | prefix.

[alexmojaki]

That's probably what it is, yes. Have this function return the full width, then deal with the length of the prefix inside the class. Remember there's both the ic | prefix (which can be configured) as well as the argPrefix.

[dawngerpony]

Let me know what you think of my new solution.

[alexmojaki]

Please give this a more descriptive name.

[dawngerpony]

Done.

[alexmojaki]

This looks a bit too broad and prone to hiding real errors. I'd prefer either checking the Python version or checking for the presence of the relevant attribute.

[dawngerpony]

Done.

[alexmojaki]

The docs https://docs.python.org/3/library/os.html#os.get_terminal_size suggest using shutil (https://docs.python.org/3/library/shutil.html#shutil.get_terminal_size) instead, and looking at that function I think that's a good idea.

[dawngerpony]

Done.

[alexmojaki]

Overall I like the approach and I appreciate the PR. The supports_param idea is clever.

[dawngerpony]

@alexmojaki thanks for the review! I've tried to implement your suggestions, which actually led to a different implementation of detect_terminal_width for Python 2.x (using a backport). Let me know what you think 🙏

[alexmojaki]

I'd prefer to have near the top of the file:

try:
    from shutil import get_terminal_size
except ImportError:
    try:
        from backports.shutil_get_terminal_size import get_terminal_size
    except ImportError:
        def get_terminal_size():
            # get COLUMNS environment variable

icecream should still work if backports.shutil_get_terminal_size isn't installed, e.g. if the user chooses to uninstall it even after installing it automatically.

[dawngerpony]

Will do it that way then 👍

[dawngerpony]

(Although in my terminal, env['COLUMNS'] isn't set, so I'm not sure which environments/OSes do set that variable)

[alexmojaki]

I don't know anything about this either, but I imagine it's a convention so that humans can easily set COLUMNS=50 when running a program and expect that many parts of the program will automatically respect that.

[alexmojaki]

Looks like the shutil version doesn't raise OSError any more. But maybe the whole thing should be wrapped in except Exception just in case something weird happens.

Also you should pass the default width as a fallback to get_terminal_size. Otherwise it's going to use its own default fallback (80 columns) when the other methods fail.

[dawngerpony]

Will do on both counts.

[alexmojaki]

This still doesn't account for the width of argPrefix, or changing the prefix with configureOutput.

[dawngerpony]

Good point, will fix.

[alexmojaki]

Started to make some suggestions then I realised the except Exception is probably dealing with that more elegantly.

[dawngerpony]

Hi @alexmojaki , sorry for the delay, work has been very busy. I have been thinking that it might be best not to enable this feature by default, at least in its first release, otherwise it could introduce breaking changes for a lot of people. So I have added some code to leave it off by default. There's now also at least one test that tests it by using a @contextmanager to set env['COLUMNS'] to a specific value. Thoughts?

NB. I'm also a little bemused by the current behaviour of my new testLiteralWithShortTerminalWidth test. I thought that when printing a literal, icecream wasn't supposed to include the "name of the variable", i.e. the following code:

ic("banana")

should print:

ic| 'banana'

When I manually run a Python script with the above code, it behaves as expected. But in my test, it actually appears to print the name of the literal along with its value:

ic| "banana banana": ('banana '
                      'banana')

Any idea what's going on?

[alexmojaki]

I have been thinking that it might be best not to enable this feature by default, at least in its first release, otherwise it could introduce breaking changes for a lot of people.

I think if the experience is generally improved then it should be enabled by default. Most people aren't going to even know they can configure this. As for breaking changes, this is a 'casual' debugging tool, I don't think there are significant expectations or guarantees of backward compatibility. It's hard to picture how this would significantly affect people negatively. If we want to be really careful, we could release a new major version. @gruns what do you think?

NB. I'm also a little bemused by the current behaviour of my new testLiteralWithShortTerminalWidth test. I thought that when printing a literal, icecream wasn't supposed to include the "name of the variable"
Any idea what's going on?

This is a bug first noticed in #84 (comment)

Basically the source of the literal argument is still printed when the value is multiline, but it shouldn't.

[alexmojaki]

The main problem is that you still need to account for the other parts of the output, and it's quite complicated. There's four pieces:

1. The global prefix (ic| )
2. The optional context (the filename, line number, etc)
3. The argPrefix (the source of the argument)
4. The value, which may not be included if the argument is a literal.

Each of these pieces needs to be added one at a time to see if they can fit within the terminal width. If not, that starts a new indented line with a new available width.

Your code mentions both 'terminal width' and 'line wrap width' in more places than I think it should, and the distinction is confusing. You should only need to store the attribute lineWrapWidth. It represents the full width for containing everything, by default the detected terminal width. The width passed to argToStringFunction should just be calculated from scratch each time _constructArgumentOutput is called. Don't calculate anything in advance based on prefix.

[dawngerpony]

The main problem is that you still need to account for the other parts of the output, and it's quite complicated. There's four pieces:

1. The global `prefix` (`ic| `)

2. The optional `context` (the filename, line number, etc)

3. The `argPrefix` (the source of the argument)

4. The value, which may not be included if the argument is a literal.

Each of these pieces needs to be added one at a time to see if they can fit within the terminal width. If not, that starts a new indented line with a new available width.

Thank you, this is the context I needed. I think I've catered for the prefix, but definitely not for anything else. I suspect that a lot more digging is needed for me to get familiar with how/when 2, 3 and 4 are added to the output.

Your code mentions both 'terminal width' and 'line wrap width' in more places than I think it should, and the distinction is confusing. You should only need to store the attribute lineWrapWidth. It represents the full width for containing everything, by default the detected terminal width. The width passed to argToStringFunction should just be calculated from scratch each time _constructArgumentOutput is called. Don't calculate anything in advance based on prefix.

OK, will do.

Also, based on your thoughts, I'll refactor to remove the "disable by default". It'll be on for everyone. Let's just hope nobody is basing important logic on the output of an informal debugging tool 😉

I'm not sure I picked the simplest task for my first contribution 😂 😬

BTW, thank you so much for sticking with me on this 🙏

[Jakeroid]

@dawngerpony

Hi,

Thank you for your efforts. Do you think you are still interested in contributing? I'm looking to clear PRs, Issues.