Remove F401 (warning for unused imports) from lint-python.sh

sdaftuar commented at 5:55 pm on November 1, 2019: member

This doesn’t seem to catch any bugs and is just an inconvenience for contributors.

Remove F401 (warning for unused imports) from lint-python.sh a2bb3706d6

sdaftuar commented at 5:55 pm on November 1, 2019: member

As requested by @thebluematt #17335 (comment)

jamesob commented at 5:58 pm on November 1, 2019: member

sure why not ACK https://github.com/bitcoin/bitcoin/pull/17346/commits/a2bb3706d618537e8507590054401d839ac3e886

DrahtBot added the label Tests on Nov 1, 2019

jnewbery commented at 6:12 pm on November 1, 2019: member

I don’t understand what the problem is here. Why not just import what you’re using?

It should be very easy to set up a linter in any text editor to flag these for you.

sdaftuar commented at 6:20 pm on November 1, 2019: member

@jnewbery I’m not proposing in this PR to remove the linter altogether, merely to remove a rule that seems to have no benefit.

MarcoFalke commented at 7:53 pm on November 1, 2019: member

ACK a2bb3706d618537e8507590054401d839ac3e886 If needed this can be added to a non-failing linter (like the typo check).

Empact commented at 9:04 pm on November 1, 2019: member

-0 I don’t see it as a significant burden to remove imports that are not used in the code.

IMO of all places, we should bias toward our code being clean.

laanwj commented at 8:34 am on November 2, 2019: member

Is this another case of #16961, where usage cannot be determined perfectly due to Python’s dynamic nature? Are there cases where it’s ambigious, platform dependent, etc?

practicalswift commented at 11:05 am on November 2, 2019: contributor

I find it easier to understand and reason about code if I know what modules and/or functions it depends on.

I think it is great to know that said dependency information is accurate.

The verification of the correctness of the dependency information can be done with 100% accuracy by machines during pre-review or with sub-100% accuracy by humans during review or after merge.

Seems like a pretty easy choice to me :)

As noted by @jnewbery it should be easy set up a linter in any text editor to flag these for you.

OTOH, if F401 is creating false positives/false negatives I can understand the frustration: but I don’t think that is the claim? I’ve never seen such a case IIRC.

laanwj commented at 11:06 am on November 2, 2019: member

I think it is great to know that said dependency information is accurate.

Which is why I ask. If it’s 100% accurate, I think the lint is good to have. If it’s not, like vulture, then it shouldn’t be part of automatic checks.

practicalswift commented at 11:07 am on November 2, 2019: contributor

Which is why I ask. If it’s 100% accurate, I think the lint is good to have. If it’s not, like vulture, then it shouldn’t be part of automatic checks.

Agree 100%.

promag commented at 9:27 pm on November 2, 2019: member

NACK, I subscribe @jnewbery opinion and I also think it’s worst to see PR’s removing unnecessary imports.

MarcoFalke commented at 9:38 pm on November 2, 2019: member

Ok, seems too controversial.

As a side note, it takes travis about 30 seconds to run this check. I use that time to write the pull request description and then check back for the travis result. I understand that installing and running the linter locally is a hassle, but spending half a minute on the pull request description, title and python imports shouldn’t be too much of a burden.

sdaftuar closed this on Nov 2, 2019

jnewbery commented at 10:05 pm on November 2, 2019: member

I understand that installing and running the linter locally is a hassle

This part I don’t understand. It took me less than 5 minutes to install syntastic and flake8 for vim, and I’ve saved that time many times over by having the linter catch bugs and errors before I even run a test.

sdaftuar commented at 10:24 pm on November 2, 2019: member

Is it unreasonable that different people might not have the same preferred development environment? If you’re going to tell other contributors how to work, you should have a good reason for doing so. Obviously I think this is a bad reason, and no one has really justified the benefit here aside from practicalswift’s weak defense of “understanding dependencies”; everyone else is arguing that the cost is low. Which is a terrible argument since the cost is obviously borne by other people, who I assert are not lying when they say that this is an inconvenience.

laanwj commented at 10:45 am on November 4, 2019: member

I understand that installing and running the linter locally is a hassle

Some checks are a hassle. But in this case it’s simply:

0$ bash test/lint/lint-python.sh

It even has a nice error message how to install flake8 if it isn’t installed.

Which brings me to the point: do we have simple instructions, in one of the documentation files, on how to run these checks locally, without assuming someone installs them (or can install them) into their IDE? (like: please run this if you’ve changed any python code)

who I assert are not lying when they say that this is an inconvenience.

Mostly, the idea of adding this linter was to make sure that we don’t get floods of easy PRs of people that have run the linter and found unused dependencies. Removing them is low hanging fruit but not useless enough to NACK immediately, but also detracts people from real review. One of the many of these kinds of things, unfortunately,

Otherwise we’d have to decide on a threshold on how much unused dependencies is too much, or allow it to grow indefinitely. But it requires more conscious thinking than running a checker.

jnewbery commented at 1:52 pm on November 4, 2019: member

Is it unreasonable that different people might not have the same preferred development environment? If you’re going to tell other contributors how to work…

You misunderstand me. I wouldn’t want to tell other contributors how they have to work, or how they should work. You’ve expressed frustration with the Travis python linters a few times and i was trying to suggest a method I’ve used to avoid that frustration. Sorry if it came across as dictating how you should set up your development environment.

sdaftuar commented at 7:51 pm on November 4, 2019: member

Mostly, the idea of adding this linter was to make sure that we don’t get floods of easy PRs of people that have run the linter and found unused dependencies. Removing them is low hanging fruit but not useless enough to NACK immediately, but also detracts people from real review. One of the many of these kinds of things, unfortunately, @laanwj That’s a fair point – I agree that reducing maintainer and reviewer burden from those kind of follow-on PRs is valuable as well.

MarcoFalke locked this on Dec 16, 2021

Remove F401 (warning for unused imports) from lint-python.sh #17346