avocado-framework / inspektor

Inspektor code checker
Other
11 stars 17 forks source link

inspektor: Revert the default disabled checks #23

Closed ldoktor closed 7 years ago

ldoktor commented 7 years ago

The default disabled errors used to be:

W,R,C,E1002,E1101,E1103,E1120,F0401,I0011

but when the --disable option was introduced it was changed to:

C,E,R,W,F0401,I0011

I'd be fine with most of it, but the E is IMO not really good default because wast majority of people are interested in Errors. This patch reverts to the original defaults, which might uncover a lot of errors in the projects depending on avocado as this default had lived with us for more than a year. (checked on avocado, which fails)

Signed-off-by: Lukáš Doktor ldoktor@redhat.com

ldoktor commented 7 years ago

@lmr, @clebergnu, @apahim can you please take a look at this? IMO the current default is really wrong. We could solve it by changing the travis.yml files to include our favorite defaults, but I think it'd be better to release a new inspekt and slowly adopt it in the related projects.

Note that I have no problem in changing this PR to only remove the E and then fine-tune the setting per-project, but I don't think errors should be ignored by default. This issue was spotted by @Andrei-Stepanov who was surprised his code:

#!/usr/bin/python

a = ["1", 2)

passed the ci.

ldoktor commented 7 years ago

Well actually @Andrey-Stepanov had a nice observation, that --enable has precedence in pylint order (no matter how we specify the args), so once we enable errors, we can't disable them. Given this I'd like change the default and use --enable "" as default, which includes the errors, but allows manually disabling them by --disable. What do you think?

ldoktor commented 7 years ago

Btw I'm talking about this:

[medic@w541 Desktop 1]$ inspekt lint --disable E0001 --enable W 3.py 
Pylint disabled: E0001
Pylint enabled : W
Syntax check PASS

[medic@w541 Desktop ]$ inspekt lint --disable E0001 --enable E 3.py 
Pylint disabled: E0001
Pylint enabled : E
************* Module 3
E0001:  3,0: : invalid syntax
Pylint check fail: 3.py
Syntax check FAIL

[medic@w541 Desktop 1]$ inspekt lint --disable E0001 --enable "" 3.py 
Pylint disabled: E0001
Pylint enabled : 
Syntax check PASS

[medic@w541 Desktop ]$ inspekt lint --disable E0002 --enable "" 3.py 
Pylint disabled: E0002
Pylint enabled : 
************* Module 3
E0001:  3,0: : invalid syntax
Pylint check fail: 3.py
Syntax check FAIL

Where I tried changing the pylint_args.append('--disable... lines to try changing the priority. It's not supported.

lmr commented 7 years ago

I'm not against it. The reason why by default some checks are disabled is, well, we need people to fix the issues, which takes a long time and effort on a very large code base as autotest's. Sure, I'm sorry that @AndreiStepanov found such egregious bugs, seriously I did not think the case mentioned would pass, given that it was a straight out syntax error.

So it all comes down to having people to fix errors. Slowly we can get to a point where we don't ignore any errors in avocado, but there has to be some commitment to it. As far as making the defaults to not ignore errors, that is fine.

ldoktor commented 7 years ago

Well the problem was that full E was being ignored, which means any errors. Well, thank you for accepting this.