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

Vulture #215

Merged
merged 10 commits into from
Mar 4, 2020
Merged

Vulture #215

merged 10 commits into from
Mar 4, 2020

Conversation

Itay4
Copy link
Contributor

@Itay4 Itay4 commented Feb 21, 2020

Added Vulture to linter
Running example

I've set the confidence level to 60 in order to catch things like:
image

image

Might lead to some false positives, but we can whitelist them

@Itay4 Itay4 requested review from glicht and anara123 February 21, 2020 11:38
@glicht
Copy link
Contributor

glicht commented Feb 21, 2020

I am concerned with the amount reported. Thinking of starting with 100% confidence instead of 60.

@Itay4
Copy link
Contributor Author

Itay4 commented Feb 21, 2020

@glicht with 100% confidence level we will lose the whole point of vulture, as you can see there are only 3 alerts with 100% confidence, and most of the reports are actual dead code
for example, we will miss unused functions
i think the whitelist mechanism should help us handle false positives

@glicht
Copy link
Contributor

glicht commented Feb 22, 2020

We have over 100 reported integrations/scripts. Need to discuss how we go about this.

I am not sure how easy the whitelisting is. I really don't like that they don't have inline comments. See: jendrikseipp/vulture#182 . I suggest taking one of the reported issues and creating a whitelist.

@Itay4
Copy link
Contributor Author

Itay4 commented Feb 23, 2020

@glicht please see this commit, which include unused vars and whitelisting of reports that came up in GmailSingleUser
i've used --make-whitelist to generate the whitelist automatically

@anara123
Copy link
Contributor

anara123 commented Feb 27, 2020

I think we have enough linters. Lets discuss further if we want another one

@Itay4
Copy link
Contributor Author

Itay4 commented Mar 1, 2020

@glicht updated confidence level to 100

"""
lint_files = self._get_lint_files()
python_exe = 'python2' if py_num < 3 else 'python3'
cmd_args = [python_exe, '-m', 'vulture', lint_files, '--min-confidence 100']
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's get the confidence level from an environment variable (with a default of 100). That way if a developer wants to run it with a lower confidence level she can simply set an environment variable.

@Itay4 Itay4 requested a review from glicht March 1, 2020 15:17
@Itay4 Itay4 merged commit c08f127 into master Mar 4, 2020
@Itay4 Itay4 deleted the vulture branch March 4, 2020 11:03
@RJ722
Copy link

RJ722 commented Mar 31, 2020

Hello there, vulture dev here. Just an update: Vulture now supports # noqa comments -- changes are on master, and we'd probably make a release later today/tomorrow.

@Itay4
Copy link
Contributor Author

Itay4 commented Mar 31, 2020

@RJ722 cool, we have been waiting for this feature
thanks for the update

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.

4 participants