standard-test-roles

#144 fixed standard-inventory-local code using inspekt
Merged by astepano. Opened by bgoncalv.
bgoncalv/standard-test-roles inspekt-standard-inventory-local  into  master

Download 144.patch

result running inspekt:

# inspekt indent inventory/standard-inventory-local
Indentation check PASS
# inspekt lint inventory/standard-inventory-local
Pylint disabled: W,R,C,E1002,E1101,E1103,E1120,F0401,I0011
Pylint enabled : W0611
Using config file /dev/null
Syntax check PASS
# inspekt style inventory/standard-inventory-local
PEP8 disabled: E501,E265,W601,E402
PEP8 compliance check PASS

Does inspektor ignore 80 symbols per 1 line req?

By default inspekt ignores errors E501,E265,W601,E402.

E501 - line too long (82 > 79 characters)
E265 - block comment should start with ‘# ‘
W601 - .has_key() is deprecated, use ‘in’
E402 - module level import not at top of file

If you want we can enable all warning and error using "--disable None".

# inspekt style --disable None inventory/standard-inventory-local 
PEP8 disabled: None
inventory/standard-inventory-local:7:80: E501 line too long (81 > 79 characters)
inventory/standard-inventory-local:10:80: E501 line too long (82 > 79 characters)
inventory/standard-inventory-local:11:80: E501 line too long (81 > 79 characters)
inventory/standard-inventory-local:14:80: E501 line too long (80 > 79 characters)
inventory/standard-inventory-local:18:80: E501 line too long (82 > 79 characters)
inventory/standard-inventory-local:19:80: E501 line too long (80 > 79 characters)
inventory/standard-inventory-local:20:80: E501 line too long (80 > 79 characters)
inventory/standard-inventory-local:45:80: E501 line too long (85 > 79 characters)
inventory/standard-inventory-local:60:80: E501 line too long (130 > 79 characters)
Style: inventory/standard-inventory-local FAIL 
PEP8 compliance FAIL

Please enable E501

E501 is up to your decision.

Avocado team ignore 79 limit also.

https://github.com/avocado-framework/avocado/blob/master/selftests/checkall#L169

run_rc style 'inspekt style --exclude=.git --disable E501,E265,W601,E402,E722'

It is up to you if we should take E501 into consideration or not.
Edited by astepano

Can we vote for this? :) I more like 120 as the limit, as from my experience 80 usually affects readability for me

I'd also prefer not having hard limit to 80.

Should we also vote on lint disabled options? By default: "Pylint disabled: W,R,C,E1002,E1101,E1103,E1120,F0401,I0011"

# inspekt lint --disable None inventory/standard-inventory-local 
Pylint disabled: None
Pylint enabled : W0611
Using config file /dev/null
************* Module standard-inventory-local
W0622: 53,0: list: Redefining built-in 'list'
C0103:  1,0: : Module name "standard-inventory-local" doesn't conform to snake_case naming style
C0111:  1,0: : Missing module docstring
C0111: 33,0: main: Missing function docstring
C0103: 45,27: main: Variable name "ex" doesn't conform to snake_case naming style
W0613: 33,9: main: Unused argument 'argv'
W0622: 58,8: list: Redefining built-in 'vars'
C0111: 53,0: list: Missing function docstring
W0612: 54,4: list: Unused variable 'rpms'
W0621: 67,9: host: Redefining name 'host' from outer scope (line 67)
C0111: 67,0: host: Missing function docstring
Lint: inventory/standard-inventory-local FAIL

                  Can we vote for this? :) I more like 120 as the limit, as from my experience 80 usually affects readability for me

@mvadkert I like 80 limit for Python code.

I would say that @bgoncalv should decide for this particular PR.

@bgoncalv what are errors if you take Avocado team line: --disable E501,E265,W601,E402,E72 ?

@bgoncalv what are errors if you take Avocado team line: --disable E501,E265,W601,E402,E72 ?

I'm not sure what E72 is, but by default it disables "E501,E265,W601,E402" which reports no error with current patch.
Edited by bgoncalv

hmm, what about ? 

W0622: 53,0: list: Redefining built-in 'list'
W0613: 33,9: main: Unused argument 'argv'
W0622: 58,8: list: Redefining built-in 'vars'
W0612: 54,4: list: Unused variable 'rpms'

?

They also do:

inspekt lint --enable W0102,W0611

                  They also do:

inspekt lint --enable W0102,W0611

The current patch also pass with these options enabled.

Are you fine with?

W0622: 53,0: list: Redefining built-in 'list'
W0613: 33,9: main: Unused argument 'argv'
W0622: 58,8: list: Redefining built-in 'vars'
W0612: 54,4: list: Unused variable 'rpms'

?

Are you fine with?
W0622: 53,0: list: Redefining built-in 'list'
W0613: 33,9: main: Unused argument 'argv'
W0622: 58,8: list: Redefining built-in 'vars'
W0612: 54,4: list: Unused variable 'rpms'

Not fine, but if we are following what Avocado team is doing they seem to be fine ignoring these cases...

I definitely do not like:

W0622: 53,0: list: Redefining built-in 'list'
W0622: 58,8: list: Redefining built-in 'vars'
W0612: 54,4: list: Unused variable 'rpms'

I will ask @ldoktor why they ignore such errors.

Edited by astepano

Regarding the long lines, I could not find an option on inspekt to set it to 120, for example.

We could usepycodestyle (inspekt calls it) directly with --max-line-length=120.

@bgoncalv after discussion with @cleber from Avocado Team, we came to conclusion that W0622 and W0612 are also important.
@bgoncalv Please fix code with enabled W0622 and W0612

rebased onto 4b133fe626f99a41ba1b4230368237f5c450d3fd

Updated.

result running inspekt:

# inspekt indent inventory/standard-inventory-local
Indentation check PASS
# inspekt lint --enable W0611,W0612,W0622 inventory/standard-inventory-local 
Pylint disabled: W,R,C,E1002,E1101,E1103,E1120,F0401,I0011
Pylint enabled : W0611,W0612,W0622
Using config file /dev/null
Syntax check PASS
# pycodestyle --max-line-length=120 inventory/standard-inventory-local
#

Commit 812de275 fixes this pull-request

Pull-Request has been merged by astepano

Pull-Request has been merged by astepano
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.