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 the ability to use the full path for exclude folder/file #310

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

Conversation

mirecl
Copy link

@mirecl mirecl commented Apr 4, 2023

Description

AS-IS:

vulture . --exclude=scripts,app.py  # run command

Currently, there’s no way to specify concrete path in exclude section, this is due to Vulture will always add * to both start and end of the string:

vulture/vulture/core.py

Lines 263 to 266 in e2e84d0

def prepare_pattern(pattern):
if not any(char in pattern for char in "*?["):
pattern = f"*{pattern}*"
return pattern

TO-BE:

vulture . --exclude=./scripts,./app.py  # run command

Now, prefixing exclude value with ./ will resolve to full specific path:

['path/to/project/scripts/*', '/path/to/project/app.py']

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.
  • I have run tox -e fix-style to format my code and checked the result with tox -e style.

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2023

Codecov Report

Merging #310 (068185c) into main (e2e84d0) will decrease coverage by 0.59%.
The diff coverage is 33.33%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #310      +/-   ##
==========================================
- Coverage   98.94%   98.36%   -0.59%     
==========================================
  Files          21       21              
  Lines         665      671       +6     
==========================================
+ Hits          658      660       +2     
- Misses          7       11       +4     
Impacted Files Coverage Δ
vulture/core.py 97.40% <33.33%> (-1.13%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mirecl
Copy link
Author

mirecl commented Apr 4, 2023

@jendrikseipp, what do you think? will it be useful for the project?

@jendrikseipp
Copy link
Owner

Thanks for the PR! It'll take me a while until I can review it properly. Two quick notes:

  • When working on the ignore mechanism, we don't want to break backwards compatibility.
  • Ideally, the ignore mechanism would be similar to other popular tools such as black and flake8.

If you can rework the PR in these directions, that'll help getting it merged. If you already check both of these, all the better :-)

@mirecl
Copy link
Author

mirecl commented Apr 11, 2023

@jendrikseipp, since this is a new feature - backwards compatibility should remain in full for option exclude files.

Option exclude files with ./ must not conflict with flake8 and black. But the current option exclude files approach definitely contradict flake8 and black:

vulture/vulture/core.py

Lines 263 to 266 in e2e84d0

def prepare_pattern(pattern):
if not any(char in pattern for char in "*?["):
pattern = f"*{pattern}*"
return pattern

@mirecl
Copy link
Author

mirecl commented May 3, 2023

@jendrikseipp, pls tell us how the progress on this PR is going:)

@mirecl
Copy link
Author

mirecl commented May 13, 2023

@jendrikseipp, what is the status of PR?

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.

Thanks for your work on this! Please also add a short changelog entry.

@@ -2,6 +2,7 @@
from fnmatch import fnmatch, fnmatchcase
from pathlib import Path
import pkgutil
import os
Copy link
Owner

Choose a reason for hiding this comment

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

Sort imports alphabetically. And use import os.path instead of import os.

@@ -261,6 +262,11 @@ def handle_syntax_error(e):

def scavenge(self, paths, exclude=None):
def prepare_pattern(pattern):
if pattern.startswith("./") is True:
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if pattern.startswith("./") is True:
if pattern.startswith("./"):

@@ -261,6 +262,11 @@ def handle_syntax_error(e):

def scavenge(self, paths, exclude=None):
def prepare_pattern(pattern):
if pattern.startswith("./") is True:
pattern = os.path.abspath(pattern)
if pattern.endswith(".py") is False:
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if pattern.endswith(".py") is False:
if os.path.isdir(pattern):
pattern = os.path.join(pattern, "*")

call_vulture(
[
"vulture",
"--exclude=./core.py,./whitelists",
Copy link
Owner

Choose a reason for hiding this comment

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

This test will succeed even without the exclude. I suggest to use vulture vulture/lines.py --exclude vulture/lines.py instead. And have two tests: returncode == 0 with exclude, returncode != 0 without exclude.

@jendrikseipp
Copy link
Owner

@mirecl Are you still planning to work on 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.

None yet

3 participants