Skip to content

Commit

Permalink
Merge branch '1745-python-coding-standards' of https://github.com/war…
Browse files Browse the repository at this point in the history 
…di/ckan into wardi-1745-python-coding-standards
  • Loading branch information
@amercader
amercader committed Jun 25, 2014
2 parents 0e7696c + 88f0cf9 commit e5d6898
Show file tree
Hide file tree
Showing 4 changed files with 349 additions and 173 deletions.
163 changes: 0 additions & 163 deletions ckan/tests/test_coding_standards.py
View file
Expand Up @@ -215,169 +215,6 @@ def test_bad(self):
show_fails(msg, self.fails)


class TestImportFromCkan(object):
''' Find files using from ckan import ... style imports '''

# Ckan import file exceptions
#
# These files contain lines like `from ckan import x` This should not be
# done except from ckan.common which is written specifically to share
# external functions. When files are fixed they should be removed from
# this list.
#
# The reason for this is to try to remove as many of the circular import
# issues that exist.

CKAN_IMPORTS_BLACKLIST_FILES = [
'bin/ckan-hmg-breakdown.py',
'bin/dump-ukgov.py',
'ckan/config/middleware.py',
'ckan/controllers/error.py',
'ckan/controllers/storage.py',
'ckan/lib/authenticator.py',
'ckan/lib/munge.py',
'ckan/lib/plugins.py',
'ckan/lib/search/index.py',
'ckan/lib/search/query.py',
'ckan/lib/search/sql.py',
'ckan/logic/action/__init__.py',
'ckan/logic/action/create.py',
'ckan/logic/auth/delete.py',
'ckan/logic/auth/get.py',
'ckan/logic/auth/update.py',
'ckan/logic/schema.py',
'ckan/logic/validators.py',
'ckan/migration/versions/034_resource_group_table.py',
'ckan/migration/versions/035_harvesting_doc_versioning.py',
'ckan/model/test_user.py',
'ckan/plugins/__init__.py',
'ckan/tests/__init__.py',
'ckan/tests/ckantestplugin/ckantestplugin/__init__.py',
'ckan/tests/functional/api/base.py',
'ckan/tests/functional/api/model/test_group.py',
'ckan/tests/functional/api/model/test_licenses.py',
'ckan/tests/functional/api/model/test_package.py',
'ckan/tests/functional/api/model/test_ratings.py',
'ckan/tests/functional/api/model/test_relationships.py',
'ckan/tests/functional/api/model/test_revisions.py',
'ckan/tests/functional/api/model/test_tag.py',
'ckan/tests/functional/api/test_api.py',
'ckan/tests/functional/api/test_follow.py',
'ckan/tests/functional/api/test_misc.py',
'ckan/tests/functional/api/test_package_search.py',
'ckan/tests/functional/api/test_resource.py',
'ckan/tests/functional/api/test_resource_search.py',
'ckan/tests/functional/api/test_revision_search.py',
'ckan/tests/functional/api/test_user.py',
'ckan/tests/functional/api/test_util.py',
'ckan/tests/functional/base.py',
'ckan/tests/functional/test_activity.py',
'ckan/tests/functional/test_admin.py',
'ckan/tests/functional/test_cors.py',
'ckan/tests/functional/test_follow.py',
'ckan/tests/functional/test_group.py',
'ckan/tests/functional/test_home.py',
'ckan/tests/functional/test_package.py',
'ckan/tests/functional/test_package_relationships.py',
'ckan/tests/functional/test_pagination.py',
'ckan/tests/functional/test_revision.py',
'ckan/tests/functional/test_search.py',
'ckan/tests/functional/test_storage.py',
'ckan/tests/functional/test_tag.py',
'ckan/tests/functional/test_tag_vocab.py',
'ckan/tests/functional/test_upload.py',
'ckan/tests/functional/test_user.py',
'ckan/tests/lib/__init__.py',
'ckan/tests/lib/test_alphabet_pagination.py',
'ckan/tests/lib/test_cli.py',
'ckan/tests/lib/test_dictization.py',
'ckan/tests/lib/test_dictization_schema.py',
'ckan/tests/lib/test_field_types.py',
'ckan/tests/lib/test_hash.py',
'ckan/tests/lib/test_helpers.py',
'ckan/tests/lib/test_i18n.py',
'ckan/tests/lib/test_mailer.py',
'ckan/tests/lib/test_navl.py',
'ckan/tests/lib/test_resource_search.py',
'ckan/tests/lib/test_simple_search.py',
'ckan/tests/lib/test_solr_package_search.py',
'ckan/tests/lib/test_solr_package_search_synchronous_update.py',
'ckan/tests/lib/test_solr_schema_version.py',
'ckan/tests/lib/test_solr_search_index.py',
'ckan/tests/lib/test_tag_search.py',
'ckan/tests/logic/test_action.py',
'ckan/tests/logic/test_auth.py',
'ckan/tests/logic/test_tag.py',
'ckan/tests/logic/test_validators.py',
'ckan/tests/misc/test_mock_mail_server.py',
'ckan/tests/misc/test_sync.py',
'ckan/tests/mock_mail_server.py',
'ckan/tests/mock_plugin.py',
'ckan/tests/models/test_extras.py',
'ckan/tests/models/test_group.py',
'ckan/tests/models/test_license.py',
'ckan/tests/models/test_misc.py',
'ckan/tests/models/test_package.py',
'ckan/tests/models/test_package_relationships.py',
'ckan/tests/models/test_purge_revision.py',
'ckan/tests/models/test_resource.py',
'ckan/tests/models/test_revision.py',
'ckan/tests/models/test_user.py',
'ckan/tests/pylons_controller.py',
'ckan/tests/schema/test_schema.py',
'ckan/tests/test_dumper.py',
'ckan/tests/test_plugins.py',
'ckan/tests/test_wsgi_ckanclient.py',
'ckan/websetup.py',
'ckanext/multilingual/plugin.py',
'ckanext/reclinepreview/tests/test_preview.py',
'ckanext/stats/controller.py',
'ckanext/stats/tests/__init__.py',
'ckanext/stats/tests/test_stats_lib.py',
'ckanext/stats/tests/test_stats_plugin.py',
'ckanext/test_tag_vocab_plugin.py',
'setup.py',
]
fails = {}
passes = []
done = False

@classmethod
def setup(cls):
if not cls.done:
cls.process()
cls.done = True

@classmethod
def process(cls):
blacklist = cls.CKAN_IMPORTS_BLACKLIST_FILES
re_bad_import = re.compile(r'^from\s.*\bckan\b(?!\.common).*\bimport')
for path, filename in process_directory(base_path):
f = open(path, 'r')
count = 1
errors = []
for line in f:
if re_bad_import.search(line):
errors.append('ln:%s \t%s' % (count, line[:-1]))
count += 1
if errors and not filename in blacklist:
cls.fails[filename] = output_errors(filename, errors)
elif not errors and filename in blacklist:
cls.passes.append(filename)

def test_import_good(self):
msg = 'The following files passed ckan import rules'
msg += '\nThey need removing from the test blacklist'
show_passing(msg, self.passes)

def test_import_bad(self):
msg = ('The following files have ckan import issues that need'
'resolving\nThese files contain lines like `from ckan import x`'
' This should not be done except from ckan.common which is'
' written specifically to share external functions.')
show_fails(msg, self.fails)


class TestImportStar(object):
''' Find files using from xxx import * '''

Expand Down
35 changes: 25 additions & 10 deletions doc/contributing/python.rst
View file
Expand Up @@ -36,15 +36,32 @@ We also use triple single-quotes for docstrings, see `Docstrings`_.
Imports
-------

- Don't use ``from module import *`` or ``from module import name``. Instead
just ``import module`` and then access names with ``module.name``.
See `Idioms and Anti-Idioms in Python`_.
- Avoid creating circular imports by only importing modules more
specialized than the one you are editing.

You can make long module names more concise by aliasing them::
import foo.bar.baz as baz
CKAN often uses code imported into a data structure instead of
importing names directly. For example CKAN controllers only use
``get_action`` to access logic functions. This allows
customization by CKAN plugins.

and then access it with ``baz`` in your code.
.. image:: /images/ckan_importing_diagram.png
:alt: CKAN importing diagram, general modules import
more specific modules

- Don't use ``from module import *``. Instead list the names you
need explicitly::

from module import name1, name2

Use parenthesis around the names if they are longer than one line::

from module import (name1, name2, ...
name12, name13)

Most of the current CKAN code base imports just the modules and
then accesses names with ``module.name``. This allows circular
imports in some cases and may still be necessary for exsiting
code, but is not recommended for new code.

- Make all imports at the start of the file, after the module docstring.
Imports should be grouped in the following order:
Expand All @@ -53,7 +70,6 @@ Imports
2. Third-party imports
3. CKAN imports

.. _Idioms and Anti-Idioms in Python: http://docs.python.org/2/howto/doanddont.html

Logging
-------
Expand Down Expand Up @@ -83,8 +99,7 @@ String formatting
------------------

Don't use the old ``%s`` style string formatting, e.g. ``"i am a %s" % sub``.
This kind of string formatting is not helpful for internationalization and is
going away in Python 3.
This kind of string formatting is not helpful for internationalization.

Use the `new .format() method`_ instead, and give meaningful names to each
replacement field, for example::
Expand Down

0 comments on commit e5d6898

Please sign in to comment.