build: restore configure formatting, add final message for warnings #623
Conversation
silverwind
force-pushed
the
configure-warnings
branch
from
80ac5c7 to
c7c1656
Compare
silverwind
changed the title
|
fine by me, can you give us a screenshot or fixed-format paste into here? |
|
Here you go, had to reduce the font size a bit to fit everything on this tiny screen.
|
|
lgtm but I'd like signoff from @bnoordhuis too cause I know he did some refactoring of #483 in #585 and may also have suggestions for wording (I don't really like "Careful! Warnings happened" but can't think of a good alternative). |
|
I'm open to any suggestions regardind the print and its color. Also noticed I could refactor the final check by removing the msg variable, if that style is prefered. |
| prefix = '\033[1m\033[91mWARNING\033[0m' if os.isatty(1) else 'WARNING' | ||
| global warned | ||
| warned = True | ||
| prefix = '\033[1m\033[93mWARNING\033[0m' if os.isatty(1) else 'WARNING' | ||
| print('%s: %s' % (prefix, msg)) |
There was a problem hiding this comment.
I believe it's slightly more pythonic to replace the global with:
def warn(msg):
# ...
warn.warned = True
warn.warned = FalseThere was a problem hiding this comment.
Good call, I'll fix that.
silverwind
force-pushed
the
configure-warnings
branch
3 times, most recently
from
857a4b4 to
d2723a2
Compare
|
@bnoordhuis updated with your suggestions. |
|
@bnoordhuis wait, I'll push another update as I noticed this when no warnings occured: |
| @@ -297,7 +296,8 @@ auto_downloads = nodedownload.parse(options.download_list) | |||
|
|
|||
|
|
|||
| def warn(msg): | |||
| prefix = '\033[1m\033[91mWARNING\033[0m' if os.isatty(1) else 'WARNING' | |||
| warn.warned = True | |||
| prefix = '\033[1m\033[93mWARNING\033[0m' if os.isatty(1) else 'WARNING' | |||
| print('%s: %s' % (prefix, msg)) | |||
There was a problem hiding this comment.
You need to define warn.warned, else you get an AttributeError below when warn() hasn't been called.
On the topic of the commit log, can you make the first line fit in <= 50 characters? Thanks!
silverwind
force-pushed
the
configure-warnings
branch
from
075a85a to
668bde8
Compare
|
Whoops, looks like I've broken this PR. Won't let me re-open it while the branch is still active. Guess I need to open a new one, sorry. |
This is a follow-up on #483. It fixes the readability concern of #595 by restoring the options print to its previous pretty-printed form while adding a final print in case if warnings happened.
I also took the liberty to change the color of warnings from light red to light yellow, as I think that color is more appropriate and red should be reserved for errors only.