Conversation
adjordjevic-TT
requested review from
miacim,
onenezicTT and
tt-vjovanovic
as code owners
ttexalens/exceptions.py
Outdated
| class TTTimeoutError(TTException): | ||
| """Raised when an expected operation times out.""" |
There was a problem hiding this comment.
Hm... We should minimize changes to reduce LLM bloatware...
For example, this exception is base for single exception. Why do we have it? What does it bring?
Ideally, exception would carry more info than just an error message, so user can inspect it if it needs to be processed or re-raised.
Can you go through exceptions and see which ones don't make sense? Which one should have more info assigned to it?
There was a problem hiding this comment.
Sure, is it fine for all of those which we decide are too specific to default to TTException?
There was a problem hiding this comment.
Yes. It is totally fine to have one generic exception type. But others need to be more specific...
Also, it is totally fine to split this PR into more, meaning you don't have to solve everything in this PR.
tt-vjovanovic
left a comment
tt-vjovanovic
left a comment
There was a problem hiding this comment.
Even though it was vibe coded, at the end, you are responsible for this code change. If you vibe code, first review it yourself, when you are satisfied with it, send other to take a look at your changes. Then there is no need to have warning at top mentioning that it was heavily vibe coded.
| f"Section {section.name} loaded successfully to address 0x{address:08x}. Size: {len(section.data)} bytes" | ||
| ) | ||
| except Exception as e: | ||
| except (TTException, ValueError, OSError) as e: |
There was a problem hiding this comment.
Another example, do we want to allow other exception to pass through to the user?
Do we want to make exception type for ElfLoader? Where do we put boundary on what gets new exception type and what continues to use existing TTException?
2 participants
Exception refactor that essentially centralized exceptions by creating
exceptions.pyto store all of our custom exceptions. Already existing exceptions were moved there and bunch of new ones were created to make it easier to see what went wrong. Additional benefit is when testing we can target specific exceptions that we expect to get rather than passing test if any exception is raised (this already found bug intest_mailboxes_communication_lockup.pywhich essentially always passed because when failing we would raise an exception that we would catch and say that everything went fine).Also, we will have easier time identifying which exception was raised by checking exception type rather than parsing exception output.