Update vendored llnl dependency from upstream spack#1644
Update vendored llnl dependency from upstream spack#1644douglasjacobsen wants to merge 2 commits into
Conversation
Pulled from Spack commit: a534e6456a11aae3f3306a964eb60a9fb2c4ee91
There was a problem hiding this comment.
Code Review
This pull request refactors and modernizes the terminal and logging utilities in llnl/util/tty, introducing type annotations, f-strings, and standard library alternatives like shutil.get_terminal_size(), while restructuring the multiprocessing-based logging daemon for Unix and Windows. The code review highlights several improvement opportunities: a potential bug in log.py where select.select on a buffered TextIOWrapper can cause output to get stuck, a missing row-length validation in colify_table that could lead to an IndexError, redundant runtime type checks and type annotations in colify.py and __init__.py, and a redundant bool() cast in config.py.
Ramble Performance Test MetricsResults produced with commit: ba96981
|
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #1644 +/- ##
========================================
Coverage 93.29% 93.29%
========================================
Files 352 352
Lines 34181 34181
========================================
Hits 31889 31889
Misses 2292 2292 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The codecov information is because we ignore the lines in this PR, so the test covers 0% of the lines as a result. But they are actually all tested... |
Hm interesting, if we ignore the lines then I'd assume codecov should ignore that too... Oh I think it's complaining about the one-line change in config.py. |
|
Love the update, glad that it seems to be a straightforward drop-in. One question: do we want to pull from a tagged release instead of from the head? |
Pulled from Spack commit: a534e6456a11aae3f3306a964eb60a9fb2c4ee91