Eliminate mutable default arguments
The best practice in Python is not to use mutable object (i.e. list, dictionary, or instances of most classes) as value of default argument (see https://goo.gl/Kwhg0T). A hacking rule to enforce this practice is added as well. The patch was inspired by CR https://review.openstack.org/#/c/198465/ and took most of its code from it as well. That's why original copyrights are kept. Closes-Bug: #1471349 Change-Id: Ia77b1736dbc3d4e28ff2d2c3424e9cfcaefa5077
This commit is contained in:
Timur Sufiev
parent
54495c3185
commit
7023050edd
|
|
@ -12,3 +12,4 @@ Horizon Specific Commandments
|
|||
-----------------------------
|
||||
|
||||
- Read the Horizon contributing documentation at http://docs.openstack.org/developer/horizon/contributing.html
|
||||
- [M322] Method's default argument shouldn't be mutable.
|
||||
|
|
|
|||
|
|
@ -0,0 +1,41 @@
|
|||
# Copyright (c) 2015 Intel, Inc.
|
||||
# All Rights Reserved.
|
||||
#
|
||||
# Licensed under the Apache License, Version 2.0 (the "License"); you may
|
||||
# not use this file except in compliance with the License. You may obtain
|
||||
# a copy of the License at
|
||||
#
|
||||
# http://www.apache.org/licenses/LICENSE-2.0
|
||||
#
|
||||
# Unless required by applicable law or agreed to in writing, software
|
||||
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
|
||||
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
|
||||
# License for the specific language governing permissions and limitations
|
||||
# under the License.
|
||||
|
||||
import re
|
||||
|
||||
"""
|
||||
Guidelines for writing new hacking checks
|
||||
- Use only for Horizon specific tests. OpenStack general tests
|
||||
should be submitted to the common 'hacking' module.
|
||||
- Pick numbers in the range M3xx. Find the current test with
|
||||
the highest allocated number and then pick the next value.
|
||||
If nova has an N3xx code for that test, use the same number.
|
||||
- Keep the test method code in the source file ordered based
|
||||
on the M3xx value.
|
||||
- List the new rule in the top level HACKING.rst file
|
||||
- Add test cases for each new rule to /tests/unit/test_hacking.py
|
||||
"""
|
||||
|
||||
mutable_default_args = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])")
|
||||
|
||||
|
||||
def no_mutable_default_args(logical_line):
|
||||
msg = "M322: Method's default argument shouldn't be mutable!"
|
||||
if mutable_default_args.match(logical_line):
|
||||
yield (0, msg)
|
||||
|
||||
|
||||
def factory(register):
|
||||
register(no_mutable_default_args)
|
||||
|
|
@ -0,0 +1,83 @@
|
|||
# Copyright 2015 Intel, Inc.
|
||||
#
|
||||
# Licensed under the Apache License, Version 2.0 (the "License"); you may
|
||||
# not use this file except in compliance with the License. You may obtain
|
||||
# a copy of the License at
|
||||
#
|
||||
# http://www.apache.org/licenses/LICENSE-2.0
|
||||
#
|
||||
# Unless required by applicable law or agreed to in writing, software
|
||||
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
|
||||
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
|
||||
# License for the specific language governing permissions and limitations
|
||||
# under the License.
|
||||
|
||||
import mock
|
||||
import pep8
|
||||
import textwrap
|
||||
|
||||
from horizon.hacking import checks
|
||||
from horizon.test import helpers
|
||||
|
||||
|
||||
class HackingTestCase(helpers.TestCase):
|
||||
"""This class tests the hacking checks in horizon.hacking.checks by passing
|
||||
strings to the check methods like the pep8/flake8 parser would. The parser
|
||||
loops over each line in the file and then passes the parameters to the
|
||||
check method. The parameter names in the check method dictate what type of
|
||||
object is passed to the check method. The parameter types are::
|
||||
logical_line: A processed line with the following modifications:
|
||||
- Multi-line statements converted to a single line.
|
||||
- Stripped left and right.
|
||||
- Contents of strings replaced with "xxx" of same length.
|
||||
- Comments removed.
|
||||
physical_line: Raw line of text from the input file.
|
||||
lines: a list of the raw lines from the input file
|
||||
tokens: the tokens that contribute to this logical line
|
||||
line_number: line number in the input file
|
||||
total_lines: number of lines in the input file
|
||||
blank_lines: blank lines before this one
|
||||
indent_char: indentation character in this file (" " or "\t")
|
||||
indent_level: indentation (with tabs expanded to multiples of 8)
|
||||
previous_indent_level: indentation on previous line
|
||||
previous_logical: previous logical line
|
||||
filename: Path of the file being run through pep8
|
||||
When running a test on a check method the return will be False/None if
|
||||
there is no violation in the sample input. If there is an error a tuple is
|
||||
returned with a position in the line, and a message. So to check the result
|
||||
just assertTrue if the check is expected to fail and assertFalse if it
|
||||
should pass.
|
||||
"""
|
||||
# We are patching pep8 so that only the check under test is actually
|
||||
# installed.
|
||||
|
||||
@mock.patch('pep8._checks',
|
||||
{'physical_line': {}, 'logical_line': {}, 'tree': {}})
|
||||
def _run_check(self, code, checker, filename=None):
|
||||
pep8.register_check(checker)
|
||||
|
||||
lines = textwrap.dedent(code).strip().splitlines(True)
|
||||
|
||||
checker = pep8.Checker(filename=filename, lines=lines)
|
||||
checker.check_all()
|
||||
checker.report._deferred_print.sort()
|
||||
return checker.report._deferred_print
|
||||
|
||||
def _assert_has_errors(self, code, checker, expected_errors=None,
|
||||
filename=None):
|
||||
actual_errors = [e[:3] for e in
|
||||
self._run_check(code, checker, filename)]
|
||||
self.assertEqual(expected_errors or [], actual_errors)
|
||||
|
||||
def _assert_has_no_errors(self, code, checker, filename=None):
|
||||
self._assert_has_errors(code, checker, filename=filename)
|
||||
|
||||
def test_no_mutable_default_args(self):
|
||||
self.assertEqual(1, len(list(checks.no_mutable_default_args(
|
||||
"def get_info_from_bdm(virt_type, bdm, mapping=[])"))))
|
||||
|
||||
self.assertEqual(0, len(list(checks.no_mutable_default_args(
|
||||
"defined = []"))))
|
||||
|
||||
self.assertEqual(0, len(list(checks.no_mutable_default_args(
|
||||
"defined, undefined = [], {}"))))
|
||||
|
|
@ -242,7 +242,7 @@ def metadefs_namespace_get(request, namespace, resource_type=None, wrap=False):
|
|||
|
||||
|
||||
def metadefs_namespace_list(request,
|
||||
filters={},
|
||||
filters=None,
|
||||
sort_dir='asc',
|
||||
sort_key='namespace',
|
||||
marker=None,
|
||||
|
|
@ -275,6 +275,8 @@ def metadefs_namespace_list(request,
|
|||
if get_version() < 2:
|
||||
return [], False, False
|
||||
|
||||
if filters is None:
|
||||
filters = {}
|
||||
limit = getattr(settings, 'API_RESULT_LIMIT', 1000)
|
||||
page_size = utils.get_page_size(request)
|
||||
|
||||
|
|
|
|||
|
|
@ -58,7 +58,7 @@ from openstack_dashboard.test.test_data import utils as test_utils
|
|||
wsgi.WSGIRequest.__repr__ = lambda self: "<class 'django.http.HttpRequest'>"
|
||||
|
||||
|
||||
def create_stubs(stubs_to_create={}):
|
||||
def create_stubs(stubs_to_create=None):
|
||||
"""decorator to simplify setting up multiple stubs at once via mox
|
||||
|
||||
:param stubs_to_create: methods to stub in one or more modules
|
||||
|
|
@ -89,7 +89,8 @@ def create_stubs(stubs_to_create={}):
|
|||
}
|
||||
|
||||
"""
|
||||
|
||||
if stubs_to_create is None:
|
||||
stubs_to_create = {}
|
||||
if not isinstance(stubs_to_create, dict):
|
||||
raise TypeError("create_stub must be passed a dict, but a %s was "
|
||||
"given." % type(stubs_to_create).__name__)
|
||||
|
|
|
|||
Loading…
Reference in New Issue