Alias detection solution for codemods #106
Conversation
andrecsilva
force-pushed
the
pyyaml-alias
branch
from
a199aea to
f1413c5
Compare
Codecov Report
@@ Coverage Diff @@
## main #106 +/- ##
==========================================
+ Coverage 95.88% 95.94% +0.06%
==========================================
Files 62 62
Lines 2526 2565 +39
==========================================
+ Hits 2422 2461 +39
Misses 104 104
|
There was a problem hiding this comment.
Can you address in your PR description why other codemods didn't get this lovely treatment? Grepping alias in tests I'm seeing some that could be easy to add: ruamel, defusedxml, https connection, and url sandbox had some aliasing logic too. If you can address what's the plan with those or add them in this PR that would be helpful context.
| alias.evaluated_name, | ||
| ): | ||
| return (import_node, alias) | ||
| return None |
There was a problem hiding this comment.
surprised pylint nor mypy didn't complain but all returns of a func should return the same len, so this should be (None, None)
There was a problem hiding this comment.
The return type is annotated as Optional[Tuple[...]], that's the reason.
Returning (None, None) would mess with None checks since bool((None,None)) == True.
| ) | ||
| maybe_name = maybe_name or self._module_name | ||
| if maybe_name and maybe_name == self._module_name: | ||
| self.add_needed_import(self._module_name) |
There was a problem hiding this comment.
these new green lines added are the same (or almost?) in every codemod added so maybe make this a separate method to call to dedupe
There was a problem hiding this comment.
These address a very specific problem that are shared by those codemods. I don't want to add it to the api because the solution needs NameResolutionMixin which depends on ScopeProvider.
Metadata is calculated at the start of every transform on a as-need basis (libcst looks at METADATA_DEPENDENCIES for that). Integrating it to the API means every codemod would calculate the ScopeProvider metadata, making it a bit more expensive.
Maybe creating a class for those types of codemods would be a good idea, but I'd rather do that when we try to rewrite those in pure libcst.
There was a problem hiding this comment.
I agree the duplication here is not ideal. I think we need to revisit the whole concept of utility mixins for this reason, although I understand the reasons it was done this way.
| ) | ||
| maybe_name = maybe_name or self._module_name | ||
| if maybe_name and maybe_name == self._module_name: | ||
| self.add_needed_import(self._module_name) |
There was a problem hiding this comment.
I agree the duplication here is not ideal. I think we need to revisit the whole concept of utility mixins for this reason, although I understand the reasons it was done this way.
Added alias detection for harden-pyyaml, secure-tempfile,and upgrade-sslcontext-minimum-version. Enabled tests.
andrecsilva
force-pushed
the
pyyaml-alias
branch
from
61b5429 to
eed1895
Compare
Overview
Added new alias detection solution for a few codemods.
Description
Added new alias detection solution for a few codemods (
harden-pyyaml,secure-tempfile, andupgrade-ssl-context-minimum-version). This solution will detect whenever a module is aliased and prefer to use the alias for new references to module attributes. Those use a more general tool now included inNameResolutionMixin.The
harden-ruamelandhttps-connectioncodemods had working solutions. I've enabled / added new tests for these.Other codemods (
url-sandbox,use-defusedxml) will replace module calls with another call from a drop-in replacement module.Those will ignore aliases and refer to the replacement module call directly. Since we are referring a third-party module on those, I've elected not to leave the current solution to make the change more explicit.