Add NamedTuple to dataclass conversion codemod #299
Conversation
facebook-github-bot
added
the
CLA Signed
| new_bases = [] | ||
| namedtuple_base = None | ||
|
|
||
| for base_class in original_node.bases: |
There was a problem hiding this comment.
Are you sure you want original_node here? If so, worth a comment.
There was a problem hiding this comment.
Seems that the metadata dict is directly tied to the original node's bases, so updated_node.bases does not work with the QualifiedNameProvider here. Added a comment to acknowledge this 👍
| NamedTuple-specific attributes and methods. | ||
| """ | ||
|
|
||
| DESCRIPTION: str = "Convert legacy NamedTuple class declarations to Python 3.7 dataclasses." |
There was a problem hiding this comment.
minor nit, but probably best to leave out the word "legacy" here and avoid provoking any pedantry or misleading anyone, since in upstream Python NamedTuple is not (as far as I know) deprecated or discouraged in any way. In our codebases we prefer to convert NamedTuple to dataclass, but that doesn't make NamedTuple "legacy" in general.
|
|
||
| # Need to examine the original node's bases since they are directly tied to import metadata | ||
| for base_class in original_node.bases: | ||
| # Compare the base class's qualified named against the expected typing.NamedTuple |
There was a problem hiding this comment.
| # Compare the base class's qualified named against the expected typing.NamedTuple | |
| # Compare the base class' qualified name against the expected typing.NamedTuple |
There was a problem hiding this comment.
@carljm class's is legal for singular, which this is one at a time. did you mean classes' to talk about the whole operation?
There was a problem hiding this comment.
@thatch Oops, yeah I was thinking of the plural. Made the comment due to the extraneous d and the other part was an afterthought that clearly needed a bit more thought :)
| qualified_namedtuple: QualifiedName = QualifiedName( | ||
| name=f"{self.MODULE}.{self.OBJECT}", source=QualifiedNameSource.IMPORT | ||
| ) |
There was a problem hiding this comment.
This is immutable and creating it has no side effects; any reason not to just make the entire QualifiedName object a class attribute, and not have to construct it anew each time we leave a class definition?
| lpar=cst.MaybeSentinel.DEFAULT, | ||
| rpar=cst.MaybeSentinel.DEFAULT, |
There was a problem hiding this comment.
Are these two lines necessary? What fails without them?
There was a problem hiding this comment.
They account for the scenario where all of the base classes have been removed, so that we default to class Foo: instead of class Foo():
… changed immutable variable to class attribute
We only have the document for transform of libraries (https://libcst.readthedocs.io/en/latest/codemods.html#library-of-transforms) and we can automatically generate the list of available CodemodCommands to keep the document update easier. We'll cover documentation later in Milestone 3 and @josieesh can work on it at that time (It requires writing some code to generate the rst for Sphinx and we already have something similar for our lint rules.) |
Summary
Conversion of a NamedTuple class declaration to Python 3.7
dataclasses.dataclasstype as an annotation. Perform removal of unused NamedTuple import and add the import ofdataclasses.dataclassif not already imported.Test Plan
Updated test suite:
Run entire test suite: