Narrow scope of various pylint inline disables#15364
Narrow scope of various pylint inline disables#15364balloob merged 2 commits intohome-assistant:devfrom scop:narrow-pylint
Conversation
ghost
added
the
There was a problem hiding this comment.
I'm not a big fan of in-line comment because deleting a line is way faster than marking the comment and then deleting. Also, it requires more skills with sed than if the comment is on it's own line.
Also, I see no real benefit of disabling an import-error with every import than on the top level once for all imports. We can't recover from the error in the code anyway.
There was a problem hiding this comment.
Having it on the same line is different in functionality -- same line affects that line, previous line affects that entire lexical scope. I think that should override personal preferences. And I don't quite agree that the speed of deletion or ease of sed usage should be a consideration here.
Disabling all import errors at top level once doesn't seem useful to me -- if that's what desired, then the same effect can be achieved by just disabling the error globally in pylintrc. But I don't think that's a good idea.
There was a problem hiding this comment.
Having it on the same line is different in functionality -- same line affects that line, previous line affects that entire lexical scope. I think that should override personal preferences.
If you have only one try-except block somewhere then it doesn't matter if disable broad-except at the beginning of block or inside the block itself (Ok, fixing it would be the way to go). For the tests the scoping is much more sophisticated because it's needed and make sense.
And I don't quite agree that the speed of deletion or ease of sed usage should be a consideration here.
Well, it should as we disable pylint at somewhere around 500 to 1000 places. There are still left-over from the last cleaning round and new entries are sneaking in from time to time which we need to remove.
Disabling all import errors at top level once doesn't seem useful to me -- if that's what desired, then the same effect can be achieved by just disabling the error globally in pylintrc. But I don't think that's a good idea.
Disabling something like import-error for a file or globally are two different shoes.
There was a problem hiding this comment.
Even if one can put broad-except disabling inside the try, I don't think one should, because that makes it inconsistent with a lot of other cases where such tricks are not available without undesired effects wrt scoping. (Not that I agree with it, but that inconsistency adds variability to the sed use case you mention, thus also making that harder.)
For the sed use case and acceptability of file-level import-error, I'm afraid we just have to agree to disagree. Anyway just two final data points:
For the sed use case, one should also consider lines where there are multiple symbols disabled on one line, as well as addressing possibly similar flake8 noqa comments as the pylint ones being removed. So in the big picture, being able to remove a line vs edit it doesn't really matter, more investigation/work is needed anyway. And I don't think sed is a proper tool for that job.
Top level disable=import-error in a file is a blanket permission for other bad imports to sneak into that file in addition to the ones it was originally added there for. The disable might be added there because there is a problem in CI with some imports, but the top level one may hide and let other ones through that blow up in user systems at runtime.
There was a problem hiding this comment.
Looks like that httplib should be a requirement for the platform.
There was a problem hiding this comment.
librouteros is a platform requirement and doesn't seem to be disabled.
There was a problem hiding this comment.
It's a platform requirement and doesn't seem to be disabled.
There was a problem hiding this comment.
The change will no longer cover bme680.
There was a problem hiding this comment.
True, but covering it does not appear to be necessary for the pylint tests to pass.
There was a problem hiding this comment.
I think it's the least intrusive way to achieve the desired effect.
There was a problem hiding this comment.
Separating with two spaces from the statement looks like enough and aligning with the next comment seems unnecessary.
There was a problem hiding this comment.
I think it's the least intrusive way to achieve the desired effect. Keeping it all on same line would require additional noqa comment for flake8.
|
I'm fine with narrowing the scopes. I think import-error is probably fine at the top of the file, as it only happens with requirements that fail to install on every platform (like RPi specific DHT sensor lib). Merge conflict and PyLint failed on this PR. After that it can be merged. |
|
We should resolve the conflicts or close this PR. |
|
Sorry, missed the review. Conflicts fixed and rebased. |
4 participants
Description:
Narrow scope of various pylint inline disables. Mainly top-level ones, but some semi-random ones as well.
Checklist:
tox. Your PR cannot be merged unless tests pass