jendrikseipp / vulture

Find dead Python code

MIT License

3.48k stars 152 forks source link

Issue #227, Feature/pathformat, adds --absolute-paths option #230

Open mcooperman opened 4 years ago

mcooperman commented 4 years ago

Description

by invoking with --absolute paths, output of problems found will specify file names in absolute path, platform-specific when used with PyCharm, as an external tool, can now 'jump to code' on these messages by clicking on the file paths.

Checklist:

[ ] I have updated the documentation in the README.md file or my changes don't require an update.
[ ] I have added an entry in CHANGELOG.md.
[ ] I have added or adapted tests to cover my changes.

mcooperman commented 4 years ago

I have updated the documentation in the README.md file or my changes don't require an update. I have added an entry in CHANGELOG.md. I have added or adapted tests to cover my changes.

looked at flake8, which sports an extension, 'flake8_formatter_abspath' to do exactly this same thing, which suggests it is of general use... I added this to tox.ini to make use of it for flake8 errors flagged in PyCharm when using flake8 as an external tool in the Run console of PyCharm

jendrikseipp commented 4 years ago

Could you please state the steps that are necessary to use Vulture within PyCharm?

Instead of a binary flag, I think it would be better to use the same approach as flake8, i.e., a --format parameter that accepts "relative" and "absolute". Then it will be easier to add other formats in the future (e.g., file:// URIs for clickable links in GNOME Terminal or JSON output).

mcooperman commented 4 years ago

I'd like to wait with this until #226 is merged.

I had a brief look at your patch and my first impression is that it's a little more complex than it needs to be. I don't think we need abstract base classes for example. Also, I prefer "--format" over "--path-format". Also, the patch shouldn't contain any "extras" like pylint comments.

sorry - missed those #pylints - i've become so used to them ;) will make the change to --format from --path-format i'll make the format class base non-abstract, maybe default to the relative behavior i did try to respond to the request to accommodate more flexibility in the future to specify other values for formats...

jendrikseipp commented 4 years ago

Sounds good. I'll have a closer look once the other PR is merged and you merged the master branch into your branch.

jendrikseipp commented 4 years ago

226 is now merged. If you like, you can merge the master branch into your branch. No need to rebase, I'll squash all commits in the end anyway.

mcooperman commented 3 years ago

didnt see your last comment. just pushed latest commits incl removing some stray debug prints and an attempted fix for windows errors on absolute path testing. once that's done will look at the simplification.

jendrikseipp commented 1 year ago

Are you still planning to pursue this?