Fix typing errors in console.py#2845
Conversation
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for this PR; these all makes sense, except for one flagged inline.
| self.skip_log = exc_info[1].skip_logfile | ||
| except AttributeError: | ||
| pass | ||
| _, exception, _ = sys.exc_info() |
There was a problem hiding this comment.
Is there a typing-specific reason for throwing away the exception type and traceback here, and then re-constructing those a couple of lines later for the Traceback construction?
There was a problem hiding this comment.
The 3.10 docs for sys.exc_info() specify:
If no exception is being handled anywhere on the stack, a tuple containing three
Nonevalues is returned. Otherwise, the values returned are(type, value, traceback).
In the type system, this shows up as tuple[type[BaseException], BaseException, TracebackType] | tuple[None, None, None].
This means once we know the exception value is not None, the exception type is provably not None. Equally correct and more performant:
exc_type, exc_value, exc_trace = sys.exc_info()
if exc_value is None:
return
if isinstance(exc_value, BriefcaseError):
self.skip_log = exc_value.skip_logfile
trace = Traceback.extract(
exc_type, # ty:ignore[invalid-argument-type]
exc_value,
exc_trace,
show_locals=True,
)
self.stacktraces.append((label, trace))I'll write this in a moment.
src/briefcase/console.pycontained a few type errors according toty. These have been fixed.Behaviour changes:
Console.closeis called more than once on the same instance, the method will do nothing instead of throwing anAttributeError.Console.capture_stacktracehas attributeskip_logfileyet is not aBriefcaseError,skip_logfilewill be ignored. An alternative implementation more amiable to duck typing would be ahasattr + isinstancecheck.Console.capture_stacktraceis called before an exception has been thrown, it will do nothing instead of throwing anAttributeError(due to implementation details ofrichv15.0.0).sys.__stdout__ is NonethenConsole.is_interactivewill returnFalseinstead of throwing aAttributeError.I don't think these branches are taken under normal execution, so there should be no runtime impact. Corresponding tests have been added.
PR Checklist:
Assisted-by: Claude Opus 4.7