-
Notifications
You must be signed in to change notification settings - Fork 230
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
Added setting to hide the header #614
Conversation
pudb/debugger.py
Outdated
self.top._w.header = self.header if override else None | ||
self.top._w.header = self.header if not CONFIG["hide_header"] else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an else
missing here? Otherwise, it would seem the override
logic would be ineffective. (Why is it needed anyhow?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there's a return missing here between these lines. Good catch. I knew something was off with it.
The override
is just a glorified show_header
function that I squished into a single function. In hindsight there's no reason to not just have a separate function for it. The purpose was for showing exceptions since the warning for it occurs in the header and it needs to be displayed even if the header is hidden.
I'll break it up into two clearer functions and you can see how you feel about it
f54b1eb
to
d35e640
Compare
elif exc_tuple is not None: | ||
caption.extend([ | ||
(None, " "), | ||
("header warning", "[PROCESSING EXCEPTION - hit 'e' to examine]") | ||
]) | ||
self.show_header() | ||
CONFIG["hide_header"] = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little weird; at this point the on-disk configuration will start disagreeing from the in-memory one. I'm guessing if one opens the preferences dialog, this changed config will then be persisted to disk, but not otherwise, which may be confusing.
Overall, I find this code a bit too stateful. I would like it better if there were simply one place that decides whether the header is shown (if configured so, or if an there's an exception), and then all the places that may cause state changes affecting this decision call that central place.
Now you see it
Now you don't
In reference to this issue
#542