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

@@ -1,11 +1,34 @@ 

  #!/usr/bin/env python

  

+ # The MIT License (MIT)

+ #

+ # Copyright (c) 2017-2018 Red Hat Inc.

+ #

+ # Permission is hereby granted, free of charge, to any person obtaining a copy of

+ # this software and associated documentation files (the "Software"), to deal in

+ # the Software without restriction, including without limitation the rights to

+ # use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of

+ # the Software, and to permit persons to whom the Software is furnished to do so,

+ # subject to the following conditions:

+ #

+ # The above copyright notice and this permission notice shall be included in all

+ # copies or substantial portions of the Software.

+ #

+ # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

+ # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS

+ # FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR

+ # COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER

+ # IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN

+ # CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

+ #

+ # Author: Stef Walter <stefw@redhat.com>

+ 

  import argparse

  import json

  import os

- import shlex

  import sys

  

+ 

  def main(argv):

      parser = argparse.ArgumentParser(description="Inventory for local")

      parser.add_argument("--list", action="store_true", help="Verbose output")
@@ -14,9 +37,9 @@ 

  

      try:

          if opts.host:

-             data = host(opts.host)

+             data = inv_host(opts.host)

          else:

-             data = list()

+             data = inv_list()

          sys.stdout.write(json.dumps(data, indent=4, separators=(',', ': ')))

      except RuntimeError as ex:

          sys.stderr.write("{0}: {1}\n".format(os.path.basename(sys.argv[0]), str(ex)))
@@ -24,21 +47,25 @@ 

  

      return 0

  

- def list():

-     rpms = [ ]

-     hosts = [ ]

-     variables = { }

+ 

+ def inv_list():

+     hosts = []

+     variables = {}

      if os.environ.get("TEST_SUBJECTS", None) == "local":

-         vars = host("local")

-         if vars:

+         host_vars = inv_host("local")

+         if host_vars:

              hosts.append("local")

-             variables["local"] = vars

-     return { "subjects": { "hosts": hosts, "vars": { } }, "localhost": { "hosts": hosts, "vars": { } }, "_meta": { "hostvars": variables } }

+             variables["local"] = host_vars

+     return {"subjects": {"hosts": hosts, "vars": {}},

+             "localhost": {"hosts": hosts, "vars": {}},

+             "_meta": {"hostvars": variables}}

  

- def host(host):

+ 

+ def inv_host(host):

      if host == "local":

-         return { "ansible_connection": "local" }

+         return {"ansible_connection": "local"}

      return None

  

+ 

  if __name__ == '__main__':

      sys.exit(main(sys.argv))

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

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.

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.

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.

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 4b133fe

6 years ago

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 812de27 fixes this pull-request

Pull-Request has been merged by astepano

6 years ago

Pull-Request has been merged by astepano

6 years ago