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

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

mcooperman
Copy link

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
Copy link
Author

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
Copy link
Owner

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).

Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

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

Please go over the PR and make sure to include only the minimal changes that are necessary for the new feature plus documentation and tests.

Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

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

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.

@mcooperman
Copy link
Author

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
Copy link
Owner

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

@jendrikseipp
Copy link
Owner

#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.

Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

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

This pull request has become quite complicated. 376 new lines for such a simple feature is too much in my opinion. I think the same functionality could be obtained with a simpler design along the lines of this pseudo-code:

path = get path from item
if format == absolute:
    path.resolve()
print item with path to stdout

Can you slim down the pull request to this simpler code, please?

@mcooperman
Copy link
Author

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
Copy link
Owner

Are you still planning to pursue this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants