Open whummer opened 2 years ago
Hi @whummer, thanks for creating an issue!
As you said, I think tracking transitive inheritance across arbitrary modules would be quite difficult and easily defeated by the dynamic nature of python. But there is still some room for improvement - I think it could be done within a module, if not completely at least well enough that it results in safer transformations.
The remove annotations transformation is a tricky one because even if we can supress it for specific classes from the python standard library, there are many other classes that should have annotations preserved that python-minifier can't know.
I think it makes sense to have separate options for removing annotations from variables, functions or class attributes. Variable annotations are much safer to remove (in the sense of breaking arbitrary code) than function annotations, which are again much safer than class attribute annotations. The minification options are already a bit cluttered though, and coarse in that they apply to an entire module.
I'm considering a mechanism such that you can specify much more precise minification options, per namespace. Something like (pseudo-yaml):
src/__main__.py::*:
remove_annotations:
variables: True
function_arguments: True
function_return: True
class_attributes: True
src/__main__.py::MyClass:
remove_annotations:
class_attributes: False
For now though, If you want to work towards a PR to solve your problem I would suggest adding to RemoveAnnotations
to accept an argument in __init__
which enables or disables class attribute annotations.
First of all, huge thanks for providing this fantastic library! 🙌
We've discovered a small issue with type declarations and transitive class inheritance. If we use this sample code:
... it would get minified to:
... which is invalid Python code (tested under 3.8, 3.10, but also applies to other versions):
The issue is that the library is currently not able to detect transitive inheritance dependencies (for good reasons, as this is a hard problem - probably infeasible to solve in the general case, as class hierarchies may be distributed across different source files, and minification is only performed within the scope of a single source file.)
One solution is to use the
remove_annotations=False
flag to retain all annotations, but in a larger codebase it can actually be beneficial to remove type declarations, especially from function/method args.The code in question is this piece: https://github.com/dflook/python-minifier/blob/9748fabffcc5954a326f1e95697d00615a8937b5/src/python_minifier/transforms/remove_annotations.py#L100-L106 The ideal case would be if we can introduce a config flag to disable the transformation that is happening for class attributes in this block.
@dflook Would it make sense to distinguish between (1) function annotations and (2) class annotations, and introduce a new flag
remove_annotations_class
? We could then either introduce a newSuiteTransformer
for class annotations, or alternatively leave the current structure and skip executingvisit_AnnAssign
ifremove_annotations_class
isFalse
. Happy to work towards a PR, but would like to get your thoughts and guidance first.. 👍 Thanks!