Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add codemod command for removing unused imports #266

Merged
merged 3 commits into from
Mar 20, 2020

Conversation

zsol
Copy link
Member

@zsol zsol commented Mar 17, 2020

Test Plan

Ran on an earlier version of this file:

❯ python -m libcst.tool codemod remove_unused_imports.RemoveUnusedImportsCommand libcst/codemod/commands/remove_unused_imports.py -u                                                                      normal
Calculating full-repo metadata...
Executing codemod...
0.01s 0% complete, [calculating] estimated for 1 files to go...--- /home/zsol/libcst/libcst/codemod/commands/remove_unused_imports.py
+++ /home/zsol/libcst/libcst/codemod/commands/remove_unused_imports.py
@@ -3,16 +3,14 @@
 # This source code is licensed under the MIT license found in the
 # LICENSE file in the root directory of this source tree.
 #
 # pyre-strict
 
-from typing import Type, Generator
-from libcst.codemod import Codemod, VisitorBasedCodemodCommand
+from libcst.codemod import VisitorBasedCodemodCommand
 
 from libcst.codemod.visitors import RemoveImportsVisitor
 from libcst import Import, ImportFrom
-import asyncio
 
 
 class RemoveUnusedImportsCommand(VisitorBasedCodemodCommand):
     DESCRIPTION: str = "Remove all imports that are not used in a file."
 
Finished codemodding 1 files!
 - Transformed 1 files successfully.
 - Skipped 0 files.
 - Failed to codemod 0 files.
 - 0 warnings were generated.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 17, 2020
@codecov-io
Copy link

codecov-io commented Mar 17, 2020

Codecov Report

Merging #266 into master will decrease coverage by <.01%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
- Coverage   93.89%   93.89%   -0.01%     
==========================================
  Files         214      216       +2     
  Lines       20822    20854      +32     
==========================================
+ Hits        19550    19580      +30     
- Misses       1272     1274       +2
Impacted Files Coverage Δ
...demod/commands/tests/test_remove_unused_imports.py 100% <100%> (ø)
libcst/codemod/commands/remove_unused_imports.py 81.81% <81.81%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f612604...610de7e. Read the comment docs.

Copy link
Contributor

@jimmylai jimmylai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!
Can you add test cases in this directory like the other codemods?
https://github.com/Instagram/LibCST/tree/master/libcst/codemod/commands/tests

@zsol
Copy link
Member Author

zsol commented Mar 19, 2020

Done! Thanks for the review. I'm wondering if we could have some better documentation about what these built-in codemods do.

@jimmylai
Copy link
Contributor

@zsol , your change has a lint error which the updated imports are not sorted. You can tox -e autofix to fix them.
Do you mean better documentation for RemoveImportsVisitor? We have detailed documentation for each of codemod helper:
https://libcst.readthedocs.io/en/latest/codemods.html#libcst.codemod.visitors.RemoveImportsVisitor

@zsol
Copy link
Member Author

zsol commented Mar 19, 2020

Do you mean better documentation for RemoveImportsVisitor?

No I meant for the Commands themselves. It's great that they have online docs python -m libcst.tool mycodemod --help, but I'm wondering if we can somehow put those in the generate sphinx docs.

Linters fixed, sorry :)

Comment on lines +47 to +48
import a
x: a = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, RemoveImportsVisitor uses scope analysis and it doesn't analyze string type annotation.
That means, in this example code, import a will be removed by this codemod.

import a
x: "a" = 1

In general, RemoveUnusedImportsCommand should be used with cautions since it has limitations including it doesn't consider imports used by other modules (indirect imports) and it doesn't consider references in string type annotations.
@zsol, would you like to include those as warnings as document? Probably short description in DESCRIPTION attr and longer explanation in docstring of RemoveUnusedImportsCommand.

@jimmylai
Copy link
Contributor

No I meant for the Commands themselves. It's great that they have online docs python -m libcst.tool mycodemod --help, but I'm wondering if we can somehow put those in the generate sphinx docs.

We have a codemod tutorial presents usage of libcst CLI.
https://libcst.readthedocs.io/en/latest/codemods_tutorial.html
Do you think it's not clear? If you have good idea about how to restructure the document to improve readability, I'm more than happy to update it.

@zsol
Copy link
Member Author

zsol commented Mar 20, 2020

If you have good idea about how to restructure the document to improve readability, I'm more than happy to update it.

I was suggesting to generate an actual list of available codemods (along with their docs) in either the tutorial there, or the reference manual part. This would allow people to explore them without needing to install libcst. I'll look at how hard this is to do.

@zsol zsol merged commit a65c67d into Instagram:master Mar 20, 2020
@zsol zsol deleted the unused-import branch March 20, 2020 11:10
@jimmylai
Copy link
Contributor

If you have good idea about how to restructure the document to improve readability, I'm more than happy to update it.

I was suggesting to generate an actual list of available codemods (along with their docs) in either the tutorial there, or the reference manual part. This would allow people to explore them without needing to install libcst. I'll look at how hard this is to do.

That's great idea and can make it more clear about available commands. One way to do it is to generate codemod command list reStructuredText in source/conf.py, e.g. Instagram/Fixit#7

@javierhonduco
Copy link

Seems like imports that are used in f-strings are not tracked either, guess it's similar to the string type annotation issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants