Fixed some typos and a docstring #10
Conversation
1. Added Google-style docstrings. 2. Exceptions are now how they should be. 3. Added `__all__` and `annotations`. 4. Replaced `.format()` with f-strings. I don't have a Linux/MacOS machine so you should check all this. Since I have changed the exception names, this should be a breaking release, you might rename them back to original names, but these names are more accurate.
There was a problem hiding this comment.
Just take a look, some feedback:
- The code already uses single quotes. Don't change that (or make it its own PR).
- Exception names shouldn't be changed as existing code using them will break.
- Making the exceptions expose more info (such as
LibraryNotFoundError.choicesandInternalError.ret_code) should be in another PR. But I'm not with the idea now. I don't think users' code can really use that extra information. - Still need to figure out the crash in CI
cxxfilt/__init__.py
Outdated
| @@ -1,117 +1,243 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
Is there a reason this (PEP 563) is needed?
Adding this apparently also breaks the EOL'd python 3.6
|
|
||
| class CharP(ctypes.c_char_p): | ||
| pass |
There was a problem hiding this comment.
As I remember using its subclass actually cause some behavior change. Need to figure out what it was.
cxxfilt/__init__.py
Outdated
| pass | ||
| # can be subclassed from FileNotFoundError? | ||
| # see https://stackoverflow.com/a/36077407 | ||
| class LibraryNotFoundError(Error): |
There was a problem hiding this comment.
Even the error names are currently inconsistent I would avoid changing them as doing so might break existing users.
There was a problem hiding this comment.
Also, we cannot subclass from FileNotFoundError (because it wasn't) as it changes observable behavior, such as
try:
with open(input) as file:
output = cxxfilt.demangle(file.read())
except FileNotFoundError:
print(input, 'not found')
except cxxfilt.InvalidName:
print(input, 'invalid')
else:
print(output)
cxxfilt/__init__.py
Outdated
|
|
||
| class InvalidNameError(Error): | ||
| """Raised when a mangled name is an invalid | ||
| name under the GNU C++ ABI mangling rules.""" |
There was a problem hiding this comment.
GNU C++ ABI
I'm not sure if the rules are GNU-defined. Can you provide some sources?
cxxfilt/__init__.py
Outdated
| return repr(self.mangled_name) | ||
|
|
||
| def __repr__(self) -> str: | ||
| return f"{self.mangled_name} is an invalid name" |
There was a problem hiding this comment.
I think this is redundant.
{'a': 3}['b'] says KeyError: 'b' instead of KeyError: key b is missing
|
|
||
|
|
||
| class DeferedErrorDemangler(BaseDemangler): | ||
| def __init__(self, error: Exception) -> None: | ||
| self._error = error | ||
|
|
||
| def demangleb(self, mangled_name: bytes, external_only: bool = True) -> bytes: | ||
| def demangleb(self, *_) -> bytes: |
There was a problem hiding this comment.
This will break existing code that calls using keyword arguments
cxxfilt/__init__.py
Outdated
| else: | ||
| raise InternalError('Unkwon status code: {}'.format(status.value)) | ||
| raise InternalError(ret) |
There was a problem hiding this comment.
Do not move the status code handling into the InternalError. Having them all here makes it obvious how each return value is handled.
Also, the error codes aren't intended to be exposed to the user.
|
|
||
| class Error(Exception): | ||
| """Base class for all cxxfilt exceptions.""" | ||
|
|
||
| pass |
cxxfilt/__init__.py
Outdated
|
|
||
| def __init__(self, choices: list[str]) -> None: | ||
| super().__init__() | ||
| self.choices = choices |
There was a problem hiding this comment.
Let's not expose the choices list from LibraryNotFound in this PR.
No description provided.