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

Sometimes the width calculation fail #32

Merged
merged 5 commits into from
Mar 20, 2019
Merged

Conversation

FJLendinez
Copy link
Contributor

When bullet calculate the width of terminal could fail throwing this stacktrace:

    _, n = os.popen('stty size', 'r').read().split()
ValueError: not enough values to unpack (expected 2, got 0)```

Using it we asume the standard console width when it fails

Francisco Javier Lendínez Tirado added 2 commits March 19, 2019 10:09
When bullet calculate the width of terminal could fail throwing this stacktrace:

```  File ".../bullet/utils.py", line 8, in <module>
    _, n = os.popen('stty size', 'r').read().split()
ValueError: not enough values to unpack (expected 2, got 0)```

Using it we asume the standard console width when it fails
@rcfox
Copy link
Contributor

rcfox commented Mar 19, 2019

Why not use shutil.get_terminal_size()?

@FJLendinez
Copy link
Contributor Author

FJLendinez commented Mar 19, 2019

Is better to use it https://gist.github.com/jtriley/1108174 than shutil

PS: shutil.get_terminal_sized == os.terminal_size

@rcfox
Copy link
Contributor

rcfox commented Mar 19, 2019

That code looks like it was meant for Python 2. shutil.get_terminal_size() was added in Python 3.3. I haven't looked into how it's implemented, but I get the sense that it's meant to work across platforms.

@FJLendinez
Copy link
Contributor Author

Yes, i saw it, shutil is better. I will commit it

Added shutil in order to have default values
@rcfox
Copy link
Contributor

rcfox commented Mar 19, 2019

shutil.get_terminal_size() returns (columns, lines) but you're taking the lines result now. It also probably isn't necessary it call int() on the result anymore.

@FJLendinez
Copy link
Contributor Author

Right, i didn't see that.

Removed trivial int casting
@rcfox
Copy link
Contributor

rcfox commented Mar 19, 2019

You're still taking the wrong value. Should be COLUMNS, _ = ...

Change the order of arguments. Columns are now the first value
@FJLendinez
Copy link
Contributor Author

Right thanks i was clueless 😥

@rcfox
Copy link
Contributor

rcfox commented Mar 20, 2019

No worries, it happens to me all the time.

@bchao1 bchao1 merged commit eaf1fb7 into bchao1:master Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants