[chirp_devel] [PATCH 00/11] fix pylint issues

This series adds pylint support to the cpep8 scripts, then it starts fixing the issues that are revealed. By far, the majority of the changes involve adding docstrings.
Like the push to become pep8 compliant, the cpep8.py script checks a blacklist to avoid forcing immediate compliance of the entire tree.
The virtualenv setup in cpep8.sh has been updated to install the extra pacakges needed (pylint, pyserial, and pygtk to start); however, I changed it such that the packages are install only when the VENV directory is first created. The performance penalty of checking before every run is too much to bear. I have an idea for fixing that, but one step at a time.
I am continuing to tweak the pylintrc file in order to minimize the amount of useless churn while still maximizing coverage, so current results from checking the entire tree may look worse than what actually needs to be fixed.
Zachary T Welch (11): Rename .pylintc as tools/cpep8.pylintc (#159) Update pylintrc file (#159) Add pylint support to cpep8 scripts (#159) Fix pylint issues in cpep8.py (#159) Fix pylint issues in logger.py (#159) Fix pylint issues in __init__.py files (#159) Fix pylint issues in settings.py (#159) Fix pylint issues in radioreference.py (#159) Fix pylint issues with chirp/ files (#159) Fix pyinit issues for bitwise_grammar.py (#159) Fix pylint issues in bitwise.py (#159)
.hgignore | 1 + .pylintrc | 308 -------------------------------- chirp/__init__.py | 6 + chirp/bandplan.py | 7 + chirp/bandplan_au.py | 3 + chirp/bandplan_iaru_r1.py | 4 + chirp/bandplan_iaru_r2.py | 4 + chirp/bandplan_iaru_r3.py | 4 + chirp/bandplan_na.py | 4 + chirp/bitwise.py | 104 ++++++++--- chirp/bitwise_grammar.py | 32 +++- chirp/detect.py | 7 +- chirp/drivers/__init__.py | 20 +++ chirp/errors.py | 4 + chirp/logger.py | 46 +++-- chirp/memmap.py | 8 +- chirp/radioreference.py | 16 +- chirp/settings.py | 26 ++- chirp/ui/__init__.py | 4 + chirp/util.py | 4 + csvdump/__init__.py | 4 + tests/__init__.py | 18 ++ tests/unit/__init__.py | 18 ++ tools/{cpep8.manifest => cpep8.lintful} | 25 +-- tools/cpep8.py | 123 +++++++++++-- pylintrc => tools/cpep8.pylintrc | 305 ++++++++++++++++++++++--------- tools/cpep8.sh | 12 +- 27 files changed, 640 insertions(+), 477 deletions(-) delete mode 100644 .pylintrc copy tools/{cpep8.manifest => cpep8.lintful} (86%) copy pylintrc => tools/cpep8.pylintrc (51%)

# HG changeset patch # User Zachary T Welch zach@mandolincreekfarm.com # Fake Node ID 361b8cd28e517cfd103bc2e4f410623d7b7a5ca0
Rename .pylintc as tools/cpep8.pylintc (#159)
diff --git a/.pylintrc b/tools/cpep8.pylintrc similarity index 100% rename from .pylintrc rename to tools/cpep8.pylintrc

# HG changeset patch # User Zachary T Welch zach@mandolincreekfarm.com # Fake Node ID 6446410c938e4fc3816841e508e8f06bc414f5d0
Update pylintrc file (#159)
This patch updates our pylintrc file, regenerating it with pylint 1.4.1. It also disables a few tests, adding documentation with a brief explanation for each test.
diff --git a/tools/cpep8.pylintrc b/tools/cpep8.pylintrc index 8a8f0a3..bb41c6d 100644 --- a/tools/cpep8.pylintrc +++ b/tools/cpep8.pylintrc @@ -1,11 +1,3 @@ -# lint Python modules using external checkers. -# -# This is the main checker controling the other ones and the reports -# generation. It is itself both a raw checker and an astng checker in order -# to: -# * handle message activation / deactivation at the module level -# * handle some basic but necessary stats'data (number of classes, methods...) -# [MASTER]
# Specify a configuration file. @@ -18,183 +10,315 @@ # Profiled execution. profile=no
-# Add <file or directory> to the black list. It should be a base name, not a -# path. You may set this option multiple times. -ignore=dist -ignore=icons -ignore=.hg +# Add files or directories to the blacklist. They should be base names, not +# paths. +ignore=CVS
# Pickle collected data for later comparisons. persistent=yes
-# Set the cache size for astng objects. -cache-size=500 - # List of plugins (as comma separated values of python modules names) to load, # usually to register additional checkers. load-plugins=
+# DEPRECATED +#include-ids=no + +# DEPRECATED +#symbols=no + +# Use multiple processes to speed up Pylint. +jobs=1 + +# Allow loading of arbitrary C extensions. Extensions are imported into the +# active Python interpreter and may run arbitrary code. +unsafe-load-any-extension=no + +# A comma-separated list of package or module names from where C extensions may +# be loaded. Extensions are loading into the active Python interpreter and may +# run arbitrary code +extension-pkg-whitelist= +
[MESSAGES CONTROL]
-# Enable only checker(s) with the given id(s). This option conflicts with the -# disable-checker option -#enable-checker= +# Only show warnings with the listed confidence levels. Leave empty to show +# all. Valid levels: HIGH, INFERENCE, INFERENCE_FAILURE, UNDEFINED +confidence=
-# Enable all checker(s) except those with the given id(s). This option -# conflicts with the enable-checker option -disable-checker=SIMILARITIES +# Enable the message, report, category or checker with the given id(s). You can +# either give multiple identifier separated by comma (,) or put this option +# multiple time. See also the "--disable" option for examples. +#enable=
-# Enable all messages in the listed categories. -#enable-msg-cat= +# Disable the message, report, category or checker with the given id(s). You +# can either give multiple identifiers separated by comma (,) or put this +# option multiple times (only on the command line, not in the configuration +# file where it should appear only once).You can also use "--disable=all" to +# disable everything first and then reenable specific checks. For example, if +# you want to run only the similarities checker, you can use "--disable=all +# --enable=similarities". If you want to run only the classes checker, but have +# no Warning level messages displayed, use"--disable=all --enable=classes +# --disable=W"
-# Disable all messages in the listed categories. -#disable-msg-cat= +# CHIRP disables the following checks: +# C0103: tyrannical variable naming +# C0326: whitespace (pep8 checks this well enough) +# C0330: hanging indentation (pep8 handles this too) +# R0201: methods that could be functions +# W0142: use * or ** magic +# W0511: FIXMEs +# W0621: variable in inner scope masks another of same name in outer scope
-# Enable the message(s) with the given id(s). -#enable-msg= - -# Disable the message(s) with the given id(s). -disable-msg=W0142,C0111 +disable=C0103,C0330,C0326,R0201,W0142,W0511,W0621
[REPORTS]
-# set the output format. Available formats are text, parseable, colorized, msvs -# (visual studio) and html -output-format=text - -# Include message's id in output -include-ids=no +# Set the output format. Available formats are text, parseable, colorized, msvs +# (visual studio) and html. You can also give a reporter class, eg +# mypackage.mymodule.MyReporterClass. +output-format=colorized
# Put messages in a separate file for each module / package specified on the # command line instead of printing them on stdout. Reports (if any) will be # written in a file name "pylint_global.[txt|html]". files-output=no
-# Tells wether to display a full report or only the messages +# Tells whether to display a full report or only the messages reports=yes
# Python expression which should return a note less than 10 (10 is the highest -# note).You have access to the variables errors warning, statement which -# respectivly contain the number of errors / warnings messages and the total +# note). You have access to the variables errors warning, statement which +# respectively contain the number of errors / warnings messages and the total # number of statements analyzed. This is used by the global evaluation report -# (R0004). +# (RP0004). evaluation=10.0 - ((float(5 * error + warning + refactor + convention) / statement) * 10)
# Add a comment according to your evaluation note. This is used by the global -# evaluation report (R0004). +# evaluation report (RP0004). comment=no
-# Enable the report(s) with the given id(s). -#enable-report= - -# Disable the report(s) with the given id(s). -#disable-report= - - -# checks for : -# * doc strings -# * modules / classes / functions / methods / arguments / variables name -# * number of arguments, local variables, branchs, returns and statements in -# functions, methods -# * required module attributes -# * dangerous default values as arguments -# * redefinition of function / method / class -# * uses of the global statement -# +# Template used to display messages. This is a python new-style format string +# used to format the message information. See doc for all details +#msg-template= + + +[SIMILARITIES] + +# Minimum lines number of a similarity. +min-similarity-lines=4 + +# Ignore comments when computing similarities. +ignore-comments=yes + +# Ignore docstrings when computing similarities. +ignore-docstrings=yes + +# Ignore imports when computing similarities. +ignore-imports=no + + +[TYPECHECK] + +# Tells whether missing members accessed in mixin class should be ignored. A +# mixin class is detected if its name ends with "mixin" (case insensitive). +ignore-mixin-members=yes + +# List of module names for which member attributes should not be checked +# (useful for modules/projects where namespaces are manipulated during runtime +# and thus existing member attributes cannot be deduced by static analysis +ignored-modules= + +# List of classes names for which member attributes should not be checked +# (useful for classes with attributes dynamically set). +ignored-classes=SQLObject + +# When zope mode is activated, add a predefined set of Zope acquired attributes +# to generated-members. +zope=no + +# List of members which are set dynamically and missed by pylint inference +# system, and so shouldn't trigger E0201 when accessed. Python regular +# expressions are accepted. +generated-members=REQUEST,acl_users,aq_parent + + +[SPELLING] + +# Spelling dictionary name. Available dictionaries: none. To make it working +# install python-enchant package. +spelling-dict= + +# List of comma separated words that should not be checked. +spelling-ignore-words= + +# A path to a file that contains private dictionary; one word per line. +spelling-private-dict-file= + +# Tells whether to store unknown words to indicated private dictionary in +# --spelling-private-dict-file option instead of raising a message. +spelling-store-unknown-words=no + + [BASIC]
# Required attributes for module, separated by a comma required-attributes=
-# Regular expression which should only match functions or classes name which do -# not require a docstring -#no-docstring-rgx=__.*__ -no-docstring-rgx=.* +# List of builtins function names that should not be used, separated by a comma +bad-functions=map,filter,apply,input
-# Regular expression which should only match correct module names -module-rgx=(([a-z_][a-z0-9_]*)|([A-Z][a-zA-Z0-9]+))$ +# Good variable names which should always be accepted, separated by a comma +good-names=i,j,k,v,f,e,ex,Run,_
-# Regular expression which should only match correct module level names -const-rgx=(([A-Z_][A-Z0-9_]*)|(__.*__))$ +# Bad variable names which should always be refused, separated by a comma +bad-names=bar,baz,toto,tutu,tata
-# Regular expression which should only match correct class names -class-rgx=[A-Z_][a-zA-Z0-9]+$ +# Colon-delimited sets of names that determine each other's naming style when +# the name regexes allow several styles. +name-group=
-# Regular expression which should only match correct function names +# Include a hint for the correct naming format with invalid-name +include-naming-hint=no + +# Regular expression matching correct function names function-rgx=[a-z_][a-z0-9_]{1,30}$
-# Regular expression which should only match correct method names -method-rgx=[a-z_][a-z0-9_]{1,30}$ +# Naming hint for function names +function-name-hint=[a-z_][a-z0-9_]{2,30}$
-# Regular expression which should only match correct instance attribute names -attr-rgx=[a-z_][a-z0-9_]{1,30}$ - -# Regular expression which should only match correct argument names -argument-rgx=[a-z_][a-z0-9_]{1,30}$ - -# Regular expression which should only match correct variable names +# Regular expression matching correct variable names variable-rgx=[a-z_][a-z0-9_]{1,30}$
-# Regular expression which should only match correct list comprehension / -# generator expression variable names +# Naming hint for variable names +variable-name-hint=[a-z_][a-z0-9_]{2,30}$ + +# Regular expression matching correct constant names +const-rgx=(([A-Z_][A-Z0-9_]*)|(__.*__))$ + +# Naming hint for constant names +const-name-hint=(([A-Z_][A-Z0-9_]*)|(__.*__))$ + +# Regular expression matching correct attribute names +attr-rgx=[a-z_][a-z0-9_]{2,30}$ + +# Naming hint for attribute names +attr-name-hint=[a-z_][a-z0-9_]{2,30}$ + +# Regular expression matching correct argument names +argument-rgx=[a-z_][a-z0-9_]{1,30}$ + +# Naming hint for argument names +argument-name-hint=[a-z_][a-z0-9_]{2,30}$ + +# Regular expression matching correct class attribute names +class-attribute-rgx=([A-Za-z_][A-Za-z0-9_]{2,30}|(__.*__))$ + +# Naming hint for class attribute names +class-attribute-name-hint=([A-Za-z_][A-Za-z0-9_]{2,30}|(__.*__))$ + +# Regular expression matching correct inline iteration names inlinevar-rgx=[A-Za-z_][A-Za-z0-9_]*$
-# Good variable names which should always be accepted, separated by a comma -good-names=i,j,k,ex,Run,_,e +# Naming hint for inline iteration names +inlinevar-name-hint=[A-Za-z_][A-Za-z0-9_]*$
-# Bad variable names which should always be refused, separated by a comma -bad-names=foo,bar,baz,toto,tutu,tata +# Regular expression matching correct class names +class-rgx=[A-Z_][a-zA-Z0-9]+$
-# List of builtins function names that should not be used, separated by a comma -bad-functions=map,filter,apply,input +# Naming hint for class names +class-name-hint=[A-Z_][a-zA-Z0-9]+$
+# Regular expression matching correct module names +module-rgx=(([a-z_][a-z0-9_]*)|([A-Z][a-zA-Z0-9]+))$
-# try to find bugs in the code using type inference -# -[TYPECHECK] +# Naming hint for module names +module-name-hint=(([a-z_][a-z0-9_]*)|([A-Z][a-zA-Z0-9]+))$
-# Tells wether missing members accessed in mixin class should be ignored. A -# mixin class is detected if its name ends with "mixin" (case insensitive). -ignore-mixin-members=yes +# Regular expression matching correct method names +method-rgx=[a-z_][a-z0-9_]{2,30}$
-# When zope mode is activated, consider the acquired-members option to ignore -# access to some undefined attributes. -zope=no +# Naming hint for method names +method-name-hint=[a-z_][a-z0-9_]{2,30}$
-# List of members which are usually get through zope's acquisition mecanism and -# so shouldn't trigger E0201 when accessed (need zope=yes to be considered). -acquired-members=REQUEST,acl_users,aq_parent +# Regular expression which should only match function or class names that do +# not require a docstring. +no-docstring-rgx=__.*__|_.* + +# Minimum line length for functions/classes that require docstrings, shorter +# ones are exempt. +docstring-min-length=-1 + + +[FORMAT] + +# Maximum number of characters on a single line. +max-line-length=80 + +# Regexp for a line that is allowed to be longer than the limit. +ignore-long-lines=^\s*(# )?<?https?://\S+>?$ + +# Allow the body of an if to be on the same line as the test if there is no +# else. +single-line-if-stmt=no + +# List of optional constructs for which whitespace checking is disabled +no-space-check=trailing-comma,dict-separator + +# Maximum number of lines in a module +max-module-lines=1500 + +# String used as indentation unit. This is usually " " (4 spaces) or "\t" (1 +# tab). +indent-string=' ' + +# Number of spaces of indent required inside a hanging or continued line. +indent-after-paren=4 + +# Expected format of line ending, e.g. empty (any line ending), LF or CRLF. +expected-line-ending-format=
-# checks for -# * unused variables / imports -# * undefined variables -# * redefinition of variable from builtins or from an outer scope -# * use of variable before assigment -# [VARIABLES]
-# Tells wether we should check for unused import in __init__ files. +# Tells whether we should check for unused import in __init__ files. init-import=no
-# A regular expression matching names used for dummy variables (i.e. not used). -dummy-variables-rgx=_|dummy +# A regular expression matching the name of dummy variables (i.e. expectedly +# not used). +dummy-variables-rgx=_|dummy|foo
# List of additional names supposed to be defined in builtins. Remember that # you should avoid to define new builtins when possible. additional-builtins=
+# List of strings which can identify a callback function by name. A callback +# name must start or end with one of those strings. +callbacks=cb_,_cb + + +[LOGGING] + +# Logging modules to check that the string format arguments are in logging +# function parameter format +logging-modules=logging + + +[MISCELLANEOUS] + +# List of note tags to take in consideration, separated by a comma. +notes=FIXME,XXX,TODO +
-# checks for sign of poor/misdesign: -# * number of methods, attributes, local variables... -# * size, complexity of functions, methods -# [DESIGN]
# Maximum number of arguments for function / method -max-args=5 +max-args=8 + +# Argument names that match this expression will be ignored. Default to name +# with leading underscore +ignored-argument-names=_.*
# Maximum number of locals for function / method body max-locals=15 @@ -203,7 +327,7 @@ max-locals=15 max-returns=6
# Maximum number of branch for function / method body -max-branchs=12 +max-branches=20
# Maximum number of statements in function / method body max-statements=50 @@ -212,97 +336,55 @@ max-statements=50 max-parents=7
# Maximum number of attributes for a class (see R0902). -max-attributes=7 +max-attributes=25
# Minimum number of public methods for a class (see R0903). -min-public-methods=2 +min-public-methods=0
# Maximum number of public methods for a class (see R0904). -max-public-methods=20 +max-public-methods=40
-# checks for : -# * methods without self as first argument -# * overridden methods signature -# * access only to existant members via self -# * attributes not defined in the __init__ method -# * supported interfaces implementation -# * unreachable code -# -[CLASSES] - -# List of interface methods to ignore, separated by a comma. This is used for -# instance to not check methods defines in Zope's Interface base class. -ignore-iface-methods=isImplementedBy,deferred,extends,names,namesAndDescriptions,queryDescriptionFor,getBases,getDescriptionFor,getDoc,getName,getTaggedValue,getTaggedValueTags,isEqualOrExtendedBy,setTaggedValue,isImplementedByInstancesOf,adaptWith,is_implemented_by - -# List of method names used to declare (i.e. assign) instance attributes. -defining-attr-methods=__init__,__new__,setUp - - -# checks for -# * external modules dependencies -# * relative / wildcard imports -# * cyclic imports -# * uses of deprecated modules -# [IMPORTS]
# Deprecated modules which should not be used, separated by a comma deprecated-modules=regsub,string,TERMIOS,Bastion,rexec
# Create a graph of every (i.e. internal and external) dependencies in the -# given file (report R0402 must not be disabled) +# given file (report RP0402 must not be disabled) import-graph=
-# Create a graph of external dependencies in the given file (report R0402 must +# Create a graph of external dependencies in the given file (report RP0402 must # not be disabled) ext-import-graph=
-# Create a graph of internal dependencies in the given file (report R0402 must +# Create a graph of internal dependencies in the given file (report RP0402 must # not be disabled) int-import-graph=
-# checks for: -# * warning notes in the code like FIXME, XXX -# * PEP 263: source code with non ascii character but no encoding declaration -# -[MISCELLANEOUS] +[CLASSES]
-# List of note tags to take in consideration, separated by a comma. -notes=FIXME,XXX,TODO +# List of interface methods to ignore, separated by a comma. This is used for +# instance to not check methods defines in Zope's Interface base class. +ignore-iface-methods=isImplementedBy,deferred,extends,names,namesAndDescriptions,queryDescriptionFor,getBases,getDescriptionFor,getDoc,getName,getTaggedValue,getTaggedValueTags,isEqualOrExtendedBy,setTaggedValue,isImplementedByInstancesOf,adaptWith,is_implemented_by
+# List of method names used to declare (i.e. assign) instance attributes. +defining-attr-methods=__init__,__new__,setUp
-# checks for : -# * unauthorized constructions -# * strict indentation -# * line length -# * use of <> instead of != -# -[FORMAT] +# List of valid names for the first argument in a class method. +valid-classmethod-first-arg=cls
-# Maximum number of characters on a single line. -max-line-length=80 +# List of valid names for the first argument in a metaclass class method. +valid-metaclass-classmethod-first-arg=mcs
-# Maximum number of lines in a module -max-module-lines=1000 +# List of member names, which should be excluded from the protected access +# warning. +exclude-protected=_asdict,_fields,_replace,_source,_make
-# String used as indentation unit. This is usually " " (4 spaces) or "\t" (1 -# tab). -indent-string=' '
+[EXCEPTIONS]
-# checks for similarities and duplicated code. This computation may be -# memory / CPU intensive, so you should disable it if you experiments some -# problems. -# -[SIMILARITIES] - -# Minimum lines number of a similarity. -min-similarity-lines=4 - -# Ignore comments when computing similarities. -ignore-comments=yes - -# Ignore docstrings when computing similarities. -ignore-docstrings=yes +# Exceptions that will emit a warning when being caught. Defaults to +# "Exception" +overgeneral-exceptions=Exception

# HG changeset patch # User Zachary T Welch zach@mandolincreekfarm.com # Fake Node ID 56fbb6a0b29a1cbbc1c3a5feeb4ce501f4cedc6e
Add pylint support to cpep8 scripts (#159)
The patch extends the cpep8 scripts to include pylint. Like pep8, it imports the appropriate pylint modules and runs the checks in the same python process. It uses the pylintrc file that was updated in the last patch to control its behavior.
By default, it only checks files from the manifest that do not appear in its blacklist (tools/cpep8.lintful). If --all is given, it scans all of the files in the manifest.
It adds the --no-pep8 and --no-pylint options to limit manual testing to one set of tests or the other. The new --no-color option may be useful when the output is not a terminal (e.g. Jenkins).
The cpep8.sh script overrides the pylint save-state directory, putting the statistics into tools/cpep.pylint.d/. As with the virtualenv files, that directory can be overridden by setting TMPDIR.
Finally, it changes the verbose option to counting, so the pylint reports for files with no errors are printed only when that option appears twice.
diff --git a/.hgignore b/.hgignore index 140091a..58a8f45 100644 --- a/.hgignore +++ b/.hgignore @@ -5,4 +5,5 @@ dist build/bdist tools/cpep8.venv/ +tools/cpep8.pylint.d/ tests/logs/ diff --git a/tools/cpep8.manifest b/tools/cpep8.lintful similarity index 94% copy from tools/cpep8.manifest copy to tools/cpep8.lintful index afeef1d..efb64b6 100644 --- a/tools/cpep8.manifest +++ b/tools/cpep8.lintful @@ -1,3 +1,6 @@ +# cpep8.lintful: The list of files that do not meet pylint standards. +# DO NOT ADD NEW FILES!! Instead, fix the code to be compliant. +# Over time, this list should shrink and (eventually) be eliminated. ./chirp/__init__.py ./chirp/bandplan.py ./chirp/bandplan_au.py diff --git a/tools/cpep8.py b/tools/cpep8.py index 2bf55d0..655a17f 100755 --- a/tools/cpep8.py +++ b/tools/cpep8.py @@ -21,6 +21,8 @@ import os import sys import logging import argparse +from pylint import lint, reporters +from pylint.reporters import text import pep8
parser = argparse.ArgumentParser() @@ -28,6 +30,12 @@ parser.add_argument("-a", "--all", action="store_true", help="Check all files, ignoring blacklist") parser.add_argument("-d", "--dir", action="store", default=".", help="Root directory of source tree") +parser.add_argument("--no-color", action="store_true", + help="Do not colorize output") +parser.add_argument("--no-pep8", action="store_true", + help="Do not run pep8 checks") +parser.add_argument("--no-pylint", action="store_true", + help="Do not run pylint checks") parser.add_argument("-s", "--stats", action="store_true", help="Only show statistics") parser.add_argument("--strict", action="store_true", @@ -36,7 +44,7 @@ parser.add_argument("-S", "--scan", action="store_true", help="Scan for additional files") parser.add_argument("-u", "--update", action="store_true", help="Update manifest/blacklist files") -parser.add_argument("-v", "--verbose", action="store_true", +parser.add_argument("-v", "--verbose", action="count", default=0, help="Display list of checked files") parser.add_argument("files", metavar="file", nargs='*', help="List of files to check (if none, check all)") @@ -55,6 +63,8 @@ scriptdir = os.path.dirname(sys.argv[0]) manifest_filename = os.path.join(scriptdir, "cpep8.manifest") blacklist_filename = os.path.join(scriptdir, "cpep8.blacklist") exceptions_filename = os.path.join(scriptdir, "cpep8.exceptions") +lintful_filename = os.path.join(scriptdir, "cpep8.lintful") +pylintrc_filename = os.path.join(scriptdir, "cpep8.pylintrc")
manifest = [] if args.scan: @@ -89,24 +99,93 @@ def get_exceptions(f): ignore = None return ignore
+ +class SmartReporter(reporters.BaseReporter): + """A pylint reporter that normally pipes output to /dev/null.""" + + def __init__(self, update): + reporters.BaseReporter.__init__(self) + self.update = update + self.stats = {} + self.msgs = {} + self.queue = [] + self.reporter = args.no_color and \ + text.TextReporter() or \ + text.ColorizedTextReporter() + + def on_set_current_module(self, module, path): + self.reporter.linter = self.linter + self.reporter.on_set_current_module(module, path) + + def handle_message(self, msg): + msg_id = msg.msg_id + if msg_id not in self.stats: + self.stats[msg_id] = 0 + self.stats[msg_id] += 1 + + if msg_id not in self.msgs: + self.msgs[msg_id] = msg.msg + + self.queue.append(msg) + + def add_message(self, dummy1, dummy2, dummy3): + pass + + def _display(self, layout): + show_report = args.verbose > 1 or \ + ((not self.update or args.verbose) and + len(self.queue) > 0) + if show_report: + for msg in self.queue: + self.reporter.handle_message(msg) + self.reporter.display_results(layout) + if args.stats: + for msgid in sorted(self.stats.keys()): + print "%d %s: %s" % \ + (self.stats[msgid], msgid, self.msgs[msgid]) + + if args.update: print "Starting update of %d files" % len(manifest) bad = [] + lintful = [] for f in manifest: checker = pep8.StyleGuide(quiet=True, ignore=get_exceptions(f)) results = checker.check_files([f]) if results.total_errors: bad.append(f) + + cmdline = [f, '--rcfile=%s' % pylintrc_filename] + reporter = SmartReporter(True) + runner = lint.Run(cmdline, reporter=reporter, exit=False) + if runner.linter.msg_status > 0: + results.total_errors += 1 + lintful.append(f) + print "%s: %s" % (results.total_errors and "FAIL" or "PASS", f)
- with file(blacklist_filename, "w") as fh: - print >>fh, """\ -# cpep8.blacklist: The list of files that do not meet PEP8 standards. + do_not_edit = """\ # DO NOT ADD NEW FILES!! Instead, fix the code to be compliant. # Over time, this list should shrink and (eventually) be eliminated.""" - print >>fh, "\n".join(sorted(bad)) + + print "Updating %s" % blacklist_filename + with file(blacklist_filename, "w") as fh: + print >>fh, """\ +# cpep8.blacklist: The list of files that do not meet PEP8 standards.""" + print >>fh, do_not_edit + if len(bad) > 0: + print >>fh, "\n".join(sorted(bad)) + + print "Updating %s" % lintful_filename + with file(lintful_filename, "w") as fh: + print >>fh, """\ +# cpep8.lintful: The list of files that do not meet pylint standards.""" + print >>fh, do_not_edit + if len(lintful) > 0: + print >>fh, "\n".join(sorted(lintful))
if args.scan: + print "Updating %s" % manifest_filename with file(manifest_filename, "w") as fh: print >>fh, "\n".join(sorted(manifest)) sys.exit(0) @@ -116,17 +195,22 @@ if args.files:
# read the blacklisted source files blacklist = file_to_lines(blacklist_filename) +lintful = file_to_lines(lintful_filename)
check_list = [] +lint_list = [] for f in manifest: - if args.all or f not in blacklist: + if not args.no_pep8 and (args.all or f not in blacklist): check_list.append(f) + if not args.no_pylint and (args.all or f not in lintful): + lint_list.append(f) check_list = sorted(check_list) +lint_list = sorted(lint_list)
total_errors = 0 for f in check_list: if args.verbose: - print "Checking %s" % f + print "pep8: checking %s" % f
checker = pep8.Checker(f, quiet=args.stats, ignore=get_exceptions(f)) results = checker.check_all() @@ -134,4 +218,13 @@ for f in check_list: checker.report.print_statistics() total_errors += results
+for f in lint_list: + if args.verbose: + print "pylint: checking %s" % f + cmdline = [f, '--rcfile=%s' % pylintrc_filename] + reporter = SmartReporter(False) + runner = lint.Run(cmdline, reporter=reporter, exit=False) + if runner.linter.msg_status > 0: + total_errors += 1 + sys.exit(total_errors and 1 or 0) diff --git a/tools/cpep8.sh b/tools/cpep8.sh index 89fd9ac..938c1c6 100755 --- a/tools/cpep8.sh +++ b/tools/cpep8.sh @@ -2,9 +2,11 @@ # Runs cpep.py with the proper verion of the pep8 library.
PEP8_VERSION="1.6.2" +PYLINT_VERSION="1.4.1"
TOOLS_DIR="$(dirname $0)" VENV="${TMPDIR:-${TOOLS_DIR}}/cpep8.venv" +export PYLINTHOME="${TMPDIR:-${TOOLS_DIR}}/cpep8.pylint.d"
virtualenv="$(which virtualenv)" if [ ! -x "$virtualenv" ]; then @@ -13,9 +15,15 @@ if [ ! -x "$virtualenv" ]; then fi if [ ! -d "$VENV" ]; then virtualenv "$VENV" + source ${VENV}/bin/activate + mkdir -p ${VENV}/logs + pip install pep8==${PEP8_VERSION} >${VENV}/logs/pep8.log 2>&1 + pip install pylint==${PYLINT_VERSION} >${VENV}/logs/pylint.log 2>&1 + pip install pyserial >${VENV}/logs/pyserial.log 2>&1 + pip install pygtk >${VENV}/logs/pygtk.log 2>&1 +else + source ${VENV}/bin/activate fi
-source ${VENV}/bin/activate -pip install pep8==${PEP8_VERSION} >${VENV}/pep8.log 2>&1 ${TOOLS_DIR}/cpep8.py "$@" deactivate

# HG changeset patch # User Zachary T Welch zach@mandolincreekfarm.com # Fake Node ID 04619dcb09caa0274af44c54709234c74f3497bc
Fix pylint issues in cpep8.py (#159)
diff --git a/tools/cpep8.lintful b/tools/cpep8.lintful index efb64b6..c3a2ebe 100644 --- a/tools/cpep8.lintful +++ b/tools/cpep8.lintful @@ -144,5 +144,4 @@ ./tests/unit/test_settings.py ./tests/unit/test_shiftdialog.py ./tools/bitdiff.py -./tools/cpep8.py ./tools/img2thd72.py diff --git a/tools/cpep8.py b/tools/cpep8.py index 655a17f..6973d8c 100755 --- a/tools/cpep8.py +++ b/tools/cpep8.py @@ -17,9 +17,12 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +Checks CHIRP source code with pep8 and pylint. +""" + import os import sys -import logging import argparse from pylint import lint, reporters from pylint.reporters import text @@ -52,10 +55,10 @@ args = parser.parse_args()
def file_to_lines(name): - fh = file(name, "r") - lines = fh.read().split("\n") + """Read a file and return the lines split into a list.""" + with file(name, "r") as cfh: + lines = cfh.read().split("\n") lines.pop() - fh.close() return lines
@@ -92,9 +95,10 @@ if not args.strict: exceptions[filename] = codes
-def get_exceptions(f): +def get_exceptions(name): + """Returns the list of exceptions for the given file name.""" try: - ignore = exceptions[f] + ignore = exceptions[name] except KeyError: ignore = None return ignore

# HG changeset patch # User Zachary T Welch zach@mandolincreekfarm.com # Fake Node ID 3c190b762714618ac4489532b6bdae2d760162b2
Fix pylint issues in logger.py (#159)
In addition to style/documentation clean ups, this fixes a call to an undefined routine (set_log_verbosity) that was renamed (set_log_level).
diff --git a/chirp/logger.py b/chirp/logger.py index 6e841bc..dc4884a 100644 --- a/chirp/logger.py +++ b/chirp/logger.py @@ -25,11 +25,11 @@ import os import sys import logging import argparse -import platform -from chirp import CHIRP_VERSION +from chirp import CHIRP_VERSION, platform
def version_string(): + """Return a string containing the versions of CHIRP, OS, and Python.""" args = (CHIRP_VERSION, platform.get_platform().os_version_string(), sys.version.split()[0]) @@ -37,12 +37,15 @@ def version_string():
class VersionAction(argparse.Action): + """Defines a class for printing the version string.""" def __call__(self, parser, namespace, value, option_string=None): + """Print the version string""" print version_string() sys.exit(1)
def add_version_argument(parser): + """Add the --version argument to the given argument parser.""" parser.add_argument("--version", action=VersionAction, nargs=0, help="Print version and exit")
@@ -56,6 +59,20 @@ log_level_names = {"critical": logging.CRITICAL,
class Logger(object): + """Singleton for managing CHIRP logging + + Set CHIRP_DEBUG in the environment for early console debugging. + It can be a number or a name; otherwise, the logging level is + set to 'debug' in order to maintain backward compatibility. + + Likewise, set CHIRP_LOG in the environment to the name of a log + file. CHIRP_LOG_LEVEL controls the logging level. + + If we're on Win32 or MacOS, we don't use the console; instead, + we create 'debug.log', redirect all output there, and set the + console logging handler level to DEBUG. To test this on Linux, + set CHIRP_DEBUG_LOG in the environment. + """
log_format = '[%(asctime)s] %(name)s - %(levelname)s: %(message)s'
@@ -66,9 +83,6 @@ class Logger(object):
self.LOG = logging.getLogger(__name__)
- # Set CHIRP_DEBUG in environment for early console debugging. - # It can be a number or a name; otherwise, level is set to 'debug' - # in order to maintain backward compatibility. CHIRP_DEBUG = os.getenv("CHIRP_DEBUG") self.early_level = logging.WARNING if CHIRP_DEBUG: @@ -80,10 +94,6 @@ class Logger(object): except KeyError: self.early_level = logging.DEBUG
- # If we're on Win32 or MacOS, we don't use the console; instead, - # we create 'debug.log', redirect all output there, and set the - # console logging handler level to DEBUG. To test this on Linux, - # set CHIRP_DEBUG_LOG in the environment. console_stream = None console_format = '%(levelname)s: %(message)s' if hasattr(sys, "frozen") or not os.isatty(0) \ @@ -102,14 +112,13 @@ class Logger(object): self.console.setFormatter(logging.Formatter(console_format)) self.logger.addHandler(self.console)
- # Set CHIRP_LOG in environment to the name of log file. logname = os.getenv("CHIRP_LOG") self.logfile = None if logname is not None: self.create_log_file(logname) level = os.getenv("CHIRP_LOG_LEVEL") if level is not None: - self.set_log_verbosity(level) + self.set_log_level(level) else: self.set_log_level(logging.DEBUG)
@@ -117,10 +126,11 @@ class Logger(object): self.LOG.debug(version_string())
def create_log_file(self, name): + """Create the log file specified by name.""" if self.logfile is None: self.logname = name # always truncate the log file - with file(name, "w") as fh: + with file(name, "w") as _: pass self.logfile = logging.FileHandler(name) format_str = self.log_format @@ -130,6 +140,7 @@ class Logger(object): self.logger.error("already logging to " + self.logname)
def set_verbosity(self, level): + """Set the logging level of the console""" self.LOG.debug("verbosity=%d", level) if level > logging.CRITICAL: level = logging.CRITICAL @@ -137,12 +148,14 @@ class Logger(object): self.console.setLevel(level)
def set_log_level(self, level): + """Set the logging level of the log file""" self.LOG.debug("log level=%d", level) if level > logging.CRITICAL: level = logging.CRITICAL self.logfile.setLevel(level)
def set_log_level_by_name(self, level): + """Set the logging level of the log file using a name""" self.set_log_level(log_level_names[level])
instance = None @@ -156,6 +169,7 @@ def is_visible(level):
def add_arguments(parser): + """Adds logging arguments to the given argument parser.""" parser.add_argument("-q", "--quiet", action="count", default=0, help="Decrease verbosity") parser.add_argument("-v", "--verbose", action="count", default=0, @@ -168,6 +182,14 @@ def add_arguments(parser):
def handle_options(options): + """Handles the logging command line options. + + If --verbose or --quiet were given, calculate the new logging level + for the console. + + If --log-file was given, create the log file. If --log-level + was given, adjust the log file level. + """ logger = Logger.instance
if options.verbose or options.quiet: diff --git a/tools/cpep8.lintful b/tools/cpep8.lintful index c3a2ebe..02ac62e 100644 --- a/tools/cpep8.lintful +++ b/tools/cpep8.lintful @@ -93,7 +93,6 @@ ./chirp/elib_intl.py ./chirp/errors.py ./chirp/import_logic.py -./chirp/logger.py ./chirp/memmap.py ./chirp/platform.py ./chirp/pyPEG.py

# HG changeset patch # User Zachary T Welch zach@mandolincreekfarm.com # Fake Node ID 12e8a0df610652585645d3278866c68505abe60c
Fix pylint issues in __init__.py files (#159)
diff --git a/chirp/__init__.py b/chirp/__init__.py index 7b48821..5a204b6 100644 --- a/chirp/__init__.py +++ b/chirp/__init__.py @@ -13,6 +13,12 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +Contains all of the core CHIRP modules. + +The driver modules are located in chirp.drivers, and the GTK GUI modules +are located in chirp.ui. +""" import os import sys from glob import glob diff --git a/chirp/drivers/__init__.py b/chirp/drivers/__init__.py index ea9dd2c..c4b10ce 100644 --- a/chirp/drivers/__init__.py +++ b/chirp/drivers/__init__.py @@ -1,3 +1,23 @@ +# Copyright 2008 Dan Smith dsmith@danplanet.com +# Copyright 2015 Zachary T Welch zach@mandolincreekfarm.com +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. + +""" +Contains all of the CHIRP driver modules. +""" + import os import sys from glob import glob diff --git a/chirp/ui/__init__.py b/chirp/ui/__init__.py index a2a6f35..443df21 100644 --- a/chirp/ui/__init__.py +++ b/chirp/ui/__init__.py @@ -12,3 +12,7 @@ # # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/. + +""" +Contains modules that implement the CHIRP GUI. +""" diff --git a/csvdump/__init__.py b/csvdump/__init__.py index e418fca..ea5f0e0 100644 --- a/csvdump/__init__.py +++ b/csvdump/__init__.py @@ -14,3 +14,7 @@ # # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/. + +""" +Implements a CSV dump utility. +""" diff --git a/tests/__init__.py b/tests/__init__.py index e69de29..303aecc 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -0,0 +1,18 @@ +# Copyright 2008 Dan Smith dsmith@danplanet.com +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. + +""" +Contains the CHIRP test suite. +""" diff --git a/tests/unit/__init__.py b/tests/unit/__init__.py index e69de29..3c3fe38 100644 --- a/tests/unit/__init__.py +++ b/tests/unit/__init__.py @@ -0,0 +1,18 @@ +# Copyright 2008 Dan Smith dsmith@danplanet.com +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. + +""" +Contains the CHIRP unit tests. +""" diff --git a/tools/cpep8.lintful b/tools/cpep8.lintful index 02ac62e..00c7919 100644 --- a/tools/cpep8.lintful +++ b/tools/cpep8.lintful @@ -1,7 +1,6 @@ # cpep8.lintful: The list of files that do not meet pylint standards. # DO NOT ADD NEW FILES!! Instead, fix the code to be compliant. # Over time, this list should shrink and (eventually) be eliminated. -./chirp/__init__.py ./chirp/bandplan.py ./chirp/bandplan_au.py ./chirp/bandplan_iaru_r1.py @@ -13,7 +12,6 @@ ./chirp/chirp_common.py ./chirp/detect.py ./chirp/directory.py -./chirp/drivers/__init__.py ./chirp/drivers/alinco.py ./chirp/drivers/anytone.py ./chirp/drivers/ap510.py @@ -98,7 +96,6 @@ ./chirp/pyPEG.py ./chirp/radioreference.py ./chirp/settings.py -./chirp/ui/__init__.py ./chirp/ui/bandplans.py ./chirp/ui/bankedit.py ./chirp/ui/clone.py @@ -123,16 +120,13 @@ ./chirpc ./chirpw ./csvdump.py -./csvdump/__init__.py ./csvdump/csvapp.py ./csvdump/csvdump.py ./locale/check_parameters.py ./rpttool ./setup.py ./share/make_supported.py -./tests/__init__.py ./tests/run_tests -./tests/unit/__init__.py ./tests/unit/base.py ./tests/unit/test_bitwise.py ./tests/unit/test_chirp_common.py

diff --git a/chirp/drivers/__init__.py b/chirp/drivers/__init__.py index ea9dd2c..c4b10ce 100644 --- a/chirp/drivers/__init__.py +++ b/chirp/drivers/__init__.py
...
+""" +Contains all of the CHIRP driver modules. +"""
I'm really -1 on requiring (and adding, for that matter) completely useless docstrings to files and functions just because pylint says to. It's just noise.
Docstrings on legitimate interface methods that actually contribute useful documentation to a user are one thing. A docstring above $thing that says "this is $thing" is not useful.
--Dan

On 03/09/2015 06:52 AM, Dan Smith wrote:
diff --git a/chirp/drivers/__init__.py b/chirp/drivers/__init__.py index ea9dd2c..c4b10ce 100644 --- a/chirp/drivers/__init__.py +++ b/chirp/drivers/__init__.py
...
+""" +Contains all of the CHIRP driver modules. +"""
I'm really -1 on requiring (and adding, for that matter) completely useless docstrings to files and functions just because pylint says to. It's just noise.
Docstrings on legitimate interface methods that actually contribute useful documentation to a user are one thing. A docstring above $thing that says "this is $thing" is not useful.
I agree that many of the module-level docstrings that I added need improvement, but their presence serves as a reminder to add more information down the road.
For example, the above docstring would serve as an appropriate location for documenting the driver interface, such that new driver writers could reference it. I am by no means sufficiently familiar with the relevant abstractions to write (or even outline) said documentation, or I would have taken a stab at it.
If you would like, I could flush out said placeholders to provide a statement of what kind of documentation might eventually end up there.

On 03/09/2015 07:40 AM, Zach Welch wrote:
On 03/09/2015 06:52 AM, Dan Smith wrote:
diff --git a/chirp/drivers/__init__.py b/chirp/drivers/__init__.py index ea9dd2c..c4b10ce 100644 --- a/chirp/drivers/__init__.py +++ b/chirp/drivers/__init__.py
...
+""" +Contains all of the CHIRP driver modules. +"""
I'm really -1 on requiring (and adding, for that matter) completely useless docstrings to files and functions just because pylint says to. It's just noise.
Docstrings on legitimate interface methods that actually contribute useful documentation to a user are one thing. A docstring above $thing that says "this is $thing" is not useful.
I agree that many of the module-level docstrings that I added need improvement, but their presence serves as a reminder to add more information down the road.
I'd also like to point out that the pydoc output is much cleaner for those modules, even if it only is a single line. Without module-level docstrings, pydoc outputs the copyright header block, which is not informative at all.
In other words, these changes don't just make pylint happy, they make pydoc's output look better too.

On Mon, Mar 9, 2015 at 7:40 AM, Zach Welch zach@mandolincreekfarm.com wrote:
I agree that many of the module-level docstrings that I added need improvement, but their presence serves as a reminder to add more information down the road.
A much better reminder that a docstring needs improvement is a warning from pylint. It looks like what you've done is provided a low-quality placeholder just to shut up pylint. This kind of thing defeats the purpose of pylint. I'm failing to see the value in this change.
Tom

On 03/10/2015 08:45 PM, Tom Hayward wrote:
On Mon, Mar 9, 2015 at 7:40 AM, Zach Welch zach@mandolincreekfarm.com wrote:
I agree that many of the module-level docstrings that I added need improvement, but their presence serves as a reminder to add more information down the road.
A much better reminder that a docstring needs improvement is a warning from pylint. It looks like what you've done is provided a low-quality placeholder just to shut up pylint. This kind of thing defeats the purpose of pylint. I'm failing to see the value in this change.
That's a valid observation, but it drives at the more fundamental reason that I was working on this issue.
At the very start, I added pylint to the automatic checking script, thinking that it would be good to have it run as part of a continuous integration script. Like the pep8 check, this will elevate the quality of all future patches. Clearly, that requires all of the pylint checks to pass for a given file, or the continuous integration check will fail.
Without fixing every issue, it can't be used continuously. If it is not part of a standard and automated review/commit process, then it will not be used universally by developers, so it might as well not be used at all.
Without enabling all of the checks that we want enforced, we cannot expect new code to achieve that standard, as it will not be caught by the integration scripts. Sure, not every check needs to be turned on, so the broader question here is what standards do the CHIRP developers care to enforce. Obviously, I think docstrings should be mandatory, thus I added a placeholder.
Right now, it seems clear that no one has been running pylint at all, so its purpose -- to be used -- has already been defeated. As such, I think that getting the tree to a point that it runs cleanly -- with all of the checks that should be enabled -- does have clear and present value. In the context of improving the continuous integration process, a placeholder is tangibly better than nothing at all. Without that ulterior motive, I would agree that the change has no value.
Now, take another step back for even more perspective. I would be happy to write suitable documentation to replace the placeholder, but I think it would be unreasonable to block these patches until I do that. Instead, we should embrace iterative and incremental development. Specifically, I need time to grok things sufficiently in order to write good documentation. In the meantime, I will do what I can to move things forward, and I think automating pylint moves us forward.

Like the pep8 check, this will elevate the quality of all future patches.
I disagree. The pep8 checks (especially the ones we have enabled) enforce basic consistency and basic mechanics. Requiring every tiny and obvious utility function to have a docstring is not equivalent to quality. Writing functions that are small in scope and clearly-readable is, IMHO. Except in a public interface, the latter obviates the need for the former in almost ever case.
Clearly, that requires all of the pylint checks to pass for a given file, or the continuous integration check will fail.
Maybe this will help: I really have no intention of making passing pylint (as configured) a requirement for all patches.
Without fixing every issue, it can't be used continuously. If it is not part of a standard and automated review/commit process, then it will not be used universally by developers, so it might as well not be used at all.
Without enabling all of the checks that we want enforced, we cannot expect new code to achieve that standard, as it will not be caught by the integration scripts. Sure, not every check needs to be turned on, so the broader question here is what standards do the CHIRP developers care to enforce. Obviously, I think docstrings should be mandatory, thus I added a placeholder.
I care far more about people that write drivers, add features to drivers, or fix bugs in drivers. A lot of those contributions come from people that aren't going to tolerate a high level of obsession over things that appear trivial to them. Since I'm not going to bring myself to reject their very useful and functional patch because of such a missing thing, if I enforce it before commit, then I have to do the work to fix them up each time.
Further, I don't want to elevate the bar so high that it appears to not be worth even trying.
I appreciate what you're trying to do, and in an ideal world, we'd completely pass all static analysis everywhere all the time. I just don't think it's appropriate for a project like this.
Now, take another step back for even more perspective. I would be happy to write suitable documentation to replace the placeholder, but I think it would be unreasonable to block these patches until I do that.
Since I don't think that "useful" documentation is needed on each of the placeholder-documented things, I don't think I'm going to change my mind about the need to enforce them until I see them.
Note that I'm not saying "show me them and I'll be convinced." I'm saying I agree that these have little value. If I thought the end goal was extremely worthwhile, then the iterative approach to getting us there would make sense.
Instead, we should embrace iterative and incremental development.
Can we add pragmatic to the above list? Start with a list of pylint rules that we can all agree on and move that forward. The changes like the ones that avoid conflicts with reserved words are reasonable. Let's make those fixes and get pylint gating on those. Then we can discuss/argue about raising the bar on specific details and consider each on its own merits.
--Dan

Without enabling all of the checks that we want enforced, we cannot expect new code to achieve that standard, as it will not be caught by the integration scripts. Sure, not every check needs to be turned on, so the broader question here is what standards do the CHIRP developers care to enforce. Obviously, I think docstrings should be mandatory, thus I added a placeholder.
I care far more about people that write drivers, add features to drivers, or fix bugs in drivers. A lot of those contributions come from people that aren't going to tolerate a high level of obsession over things that appear trivial to them. Since I'm not going to bring myself to reject their very useful and functional patch because of such a missing thing, if I enforce it before commit, then I have to do the work to fix them up each time.
I agree with Dan. I'm not a college educated programmer. I don't do software coding for a living. I build most of my patches by searching through the existing drivers to see how others have done what I want to do.
I'm sort of in a holding pattern right now to see how this shakes out. The 2+ years or so that I have been contributing to CHIRP has been done as a hobby. I enjoy doing it. If it becomes more like a job, then I'll have to move on and do some of the long list of other things I could enjoy doing.
I think that if CHIRP is changed to where someone like me (without professional programming skills) can't get started like I did, then the pool of volunteer programmers will be more like a puddle. My 2 cents.
Jim KC9HI

# HG changeset patch # User Zachary T Welch zach@mandolincreekfarm.com # Fake Node ID 31b2a1456e93f038e88cade466acf0a970b79121
Fix pylint issues in settings.py (#159)
diff --git a/chirp/settings.py b/chirp/settings.py index 55571a3..50685c7 100644 --- a/chirp/settings.py +++ b/chirp/settings.py @@ -13,6 +13,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +Provides a heiarchical means of representing device settings. +""" + from chirp import chirp_common
@@ -26,7 +30,7 @@ class InternalError(Exception): pass
-class RadioSettingValue: +class RadioSettingValue(object): """Base class for a single radio setting""" def __init__(self): self._current = None @@ -35,20 +39,23 @@ class RadioSettingValue: self._mutable = True
def set_mutable(self, mutable): + """Set whether this value can be changed.""" self._mutable = mutable
def get_mutable(self): + """Return whether this value can be changed.""" return self._mutable
def changed(self): - """Returns True if the setting has been changed since init""" + """Returns True if the setting has been changed since init.""" return self._has_changed
def set_validate_callback(self, callback): + """Set the callback used to validate the value.""" self._validate_callback = callback
def set_value(self, value): - """Sets the current value, triggers changed""" + """Sets the current value, marks it changed, and validates.""" if not self.get_mutable(): raise InvalidValueError("This value is not mutable")
@@ -57,13 +64,15 @@ class RadioSettingValue: self._current = self._validate_callback(value)
def get_value(self): - """Gets the current value""" + """Gets the current value.""" return self._current
def __trunc__(self): + """Return the integer portion of the value.""" return int(self.get_value())
def __str__(self): + """Return the string representation of the value.""" return str(self.get_value())
@@ -207,6 +216,7 @@ class RadioSettingValueString(RadioSettingValue):
class RadioSettings(list): + """Represents the top-level list of settings.""" def __init__(self, *groups): list.__init__(self, groups)
@@ -216,7 +226,7 @@ class RadioSettings(list):
class RadioSettingGroup(object): - """A group of settings""" + """Represents a group of settings.""" def _validate(self, element): # RadioSettingGroup can only contain RadioSettingGroup objects if not isinstance(element, RadioSettingGroup): @@ -259,7 +269,7 @@ class RadioSettingGroup(object): self[element.get_name()] = element
def __iter__(self): - class RSGIterator: + class RSGIterator(object): """Iterator for a RadioSettingGroup"""
def __init__(self, rsg): @@ -312,12 +322,16 @@ class RadioSetting(RadioSettingGroup): self._apply_callback = None
def set_apply_callback(self, callback, *args): + """Set the apply callback to call the given function with the + provided arguments.""" self._apply_callback = lambda: callback(self, *args)
def has_apply_callback(self): + """Returns True if the apply callback has been set.""" return self._apply_callback is not None
def run_apply_callback(self): + """Invokes the apply callback.""" return self._apply_callback()
def _validate(self, value): diff --git a/tools/cpep8.lintful b/tools/cpep8.lintful index 00c7919..49f08b2 100644 --- a/tools/cpep8.lintful +++ b/tools/cpep8.lintful @@ -95,7 +95,6 @@ ./chirp/platform.py ./chirp/pyPEG.py ./chirp/radioreference.py -./chirp/settings.py ./chirp/ui/bandplans.py ./chirp/ui/bankedit.py ./chirp/ui/clone.py

# HG changeset patch # User Zachary T Welch zach@mandolincreekfarm.com # Fake Node ID 5ff873af885be055cb879ad53186a3eb9f46dff0
Fix pylint issues in radioreference.py (#159)
diff --git a/chirp/radioreference.py b/chirp/radioreference.py index d52284e..8d4fc6c 100644 --- a/chirp/radioreference.py +++ b/chirp/radioreference.py @@ -13,6 +13,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +Network driver for connecting with radioreference.com. +""" + import logging from chirp import chirp_common, errors
@@ -82,9 +86,9 @@ class RadioReferenceRadio(chirp_common.NetworkSourceRadio): status.max += len(county.agencyList)
for cat in county.cats: - LOG.debug("Fetching category:", cat.cName) + LOG.debug("Fetching category: %s", cat.cName) for subcat in cat.subcats: - LOG.debug("\t", subcat.scName) + LOG.debug("\t%s", subcat.scName) result = self._client.service.getSubcatFreqs(subcat.scid, self._auth) self._freqs += result @@ -96,9 +100,9 @@ class RadioReferenceRadio(chirp_common.NetworkSourceRadio): for cat in agency.cats: status.max += len(cat.subcats) for cat in agency.cats: - LOG.debug("Fetching category:", cat.cName) + LOG.debug("Fetching category: %s", cat.cName) for subcat in cat.subcats: - LOG.debug("\t", subcat.scName) + LOG.debug("\t%s", subcat.scName) result = self._client.service.getSubcatFreqs(subcat.scid, self._auth) self._freqs += result @@ -141,7 +145,7 @@ class RadioReferenceRadio(chirp_common.NetworkSourceRadio): else: try: tone, tmode = freq.tone.split(" ") - except Exception: + except ValueError: tone, tmode = None, None if tmode == "PL": mem.tmode = "TSQL" @@ -150,7 +154,7 @@ class RadioReferenceRadio(chirp_common.NetworkSourceRadio): mem.tmode = "DTCS" mem.dtcs = int(tone) else: - LOG.error("Error: unsupported tone: %s" % freq) + LOG.error("Error: unsupported tone: %s", freq) try: mem.mode = self._get_mode(freq.mode) except KeyError: diff --git a/tools/cpep8.lintful b/tools/cpep8.lintful index 49f08b2..15d6174 100644 --- a/tools/cpep8.lintful +++ b/tools/cpep8.lintful @@ -94,7 +94,6 @@ ./chirp/memmap.py ./chirp/platform.py ./chirp/pyPEG.py -./chirp/radioreference.py ./chirp/ui/bandplans.py ./chirp/ui/bankedit.py ./chirp/ui/clone.py

# HG changeset patch # User Zachary T Welch zach@mandolincreekfarm.com # Fake Node ID c8b271a86a8ee9ebd4c37073eedef7e0f19b0a98
Fix pylint issues with chirp/ files (#159)
diff --git a/chirp/bandplan.py b/chirp/bandplan.py index 97f2425..37dac30 100644 --- a/chirp/bandplan.py +++ b/chirp/bandplan.py @@ -13,10 +13,15 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +Provides the Band class to use for defining band plans. +""" + from chirp import chirp_common
class Band(object): + """Represents a single band allocation.""" def __init__(self, limits, name, mode=None, step_khz=None, input_offset=None, output_offset=None, tones=None): # Apply semantic and chirp limitations to settings. @@ -56,10 +61,12 @@ class Band(object): other.limits[1] == self.limits[1])
def contains(self, other): + """Return True if the other band exists inside this band.""" return (other.limits[0] >= self.limits[0] and other.limits[1] <= self.limits[1])
def width(self): + """Return the band width.""" return self.limits[1] - self.limits[0]
def inverse(self): diff --git a/chirp/bandplan_au.py b/chirp/bandplan_au.py index 9f4ba50..e025f39 100644 --- a/chirp/bandplan_au.py +++ b/chirp/bandplan_au.py @@ -13,6 +13,9 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +The band plan for Australia. +"""
from chirp import bandplan, bandplan_iaru_r3
diff --git a/chirp/bandplan_iaru_r1.py b/chirp/bandplan_iaru_r1.py index e41b0ee..5e48b49 100644 --- a/chirp/bandplan_iaru_r1.py +++ b/chirp/bandplan_iaru_r1.py @@ -13,6 +13,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +The band plan for IARU Region 1. +""" + from chirp import bandplan
SHORTNAME = "iaru_r1" diff --git a/chirp/bandplan_iaru_r2.py b/chirp/bandplan_iaru_r2.py index 5886187..ce67334 100644 --- a/chirp/bandplan_iaru_r2.py +++ b/chirp/bandplan_iaru_r2.py @@ -13,6 +13,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +The band plan for IARU Region 2. +""" + from chirp import bandplan
SHORTNAME = "iaru_r2" diff --git a/chirp/bandplan_iaru_r3.py b/chirp/bandplan_iaru_r3.py index 08bb56e..a72a613 100644 --- a/chirp/bandplan_iaru_r3.py +++ b/chirp/bandplan_iaru_r3.py @@ -13,6 +13,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +The band plan for IARU Region 3. +""" + from chirp import bandplan
SHORTNAME = "iaru_r3" diff --git a/chirp/bandplan_na.py b/chirp/bandplan_na.py index 50bbb27..1e5d746 100644 --- a/chirp/bandplan_na.py +++ b/chirp/bandplan_na.py @@ -13,6 +13,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +The band plan for North America. +""" + from chirp import bandplan, bandplan_iaru_r2
diff --git a/chirp/detect.py b/chirp/detect.py index 7f3ee87..2a0821d 100644 --- a/chirp/detect.py +++ b/chirp/detect.py @@ -13,6 +13,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +Automatic detection of connected radio. +""" + import serial import logging
@@ -80,8 +84,7 @@ def detect_icom_radio(port):
ser.close()
- LOG.info("Auto-detected %s %s on %s" % - (result.VENDOR, result.MODEL, port)) + LOG.info("Auto-detected %s %s on %s", result.VENDOR, result.MODEL, port)
return result
diff --git a/chirp/errors.py b/chirp/errors.py index f4d9324..d64b62b 100644 --- a/chirp/errors.py +++ b/chirp/errors.py @@ -13,6 +13,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +Defines exceptions used throughout CHIRP. +""" +
class InvalidDataError(Exception): """The radio driver encountered some invalid data""" diff --git a/chirp/memmap.py b/chirp/memmap.py index af0706c..ed1953f 100644 --- a/chirp/memmap.py +++ b/chirp/memmap.py @@ -13,10 +13,14 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +Represent a device's memory map. +""" + from chirp import util
-class MemoryMap: +class MemoryMap(object): """ A pythonic memory map interface """ @@ -79,7 +83,7 @@ class MemoryMap: return self.get_packed()
def __repr__(self): - return self.printable(printit=False) + return self.printable()
def truncate(self, size): """Truncate the memory map to @size""" diff --git a/chirp/util.py b/chirp/util.py index 1fc5529..e5dd31c 100644 --- a/chirp/util.py +++ b/chirp/util.py @@ -13,6 +13,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +Assorted utility routines used throughout CHIRP. +""" + import struct
diff --git a/tools/cpep8.lintful b/tools/cpep8.lintful index 15d6174..68c32b7 100644 --- a/tools/cpep8.lintful +++ b/tools/cpep8.lintful @@ -1,16 +1,9 @@ # cpep8.lintful: The list of files that do not meet pylint standards. # DO NOT ADD NEW FILES!! Instead, fix the code to be compliant. # Over time, this list should shrink and (eventually) be eliminated. -./chirp/bandplan.py -./chirp/bandplan_au.py -./chirp/bandplan_iaru_r1.py -./chirp/bandplan_iaru_r2.py -./chirp/bandplan_iaru_r3.py -./chirp/bandplan_na.py ./chirp/bitwise.py ./chirp/bitwise_grammar.py ./chirp/chirp_common.py -./chirp/detect.py ./chirp/directory.py ./chirp/drivers/alinco.py ./chirp/drivers/anytone.py @@ -89,9 +82,7 @@ ./chirp/drivers/wouxun_common.py ./chirp/drivers/yaesu_clone.py ./chirp/elib_intl.py -./chirp/errors.py ./chirp/import_logic.py -./chirp/memmap.py ./chirp/platform.py ./chirp/pyPEG.py ./chirp/ui/bandplans.py @@ -113,7 +104,6 @@ ./chirp/ui/reporting.py ./chirp/ui/settingsedit.py ./chirp/ui/shiftdialog.py -./chirp/util.py ./chirp/xml_ll.py ./chirpc ./chirpw

# HG changeset patch # User Zachary T Welch zach@mandolincreekfarm.com # Fake Node ID df805eb669f786e63cbad9f508ffe0f0d1bfb969
Fix pyinit issues for bitwise_grammar.py (#159)
diff --git a/chirp/bitwise_grammar.py b/chirp/bitwise_grammar.py index b6eb20c..96a9b0a 100644 --- a/chirp/bitwise_grammar.py +++ b/chirp/bitwise_grammar.py @@ -13,6 +13,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/.
+""" +Grammar for parsing CHIRP's memory layout definitions. +""" + import re from chirp.pyPEG import keyword, parse as pypeg_parse
@@ -23,104 +27,130 @@ DIRECTIVES = ["seekto", "seek", "printoffset"]
def string(): + """Return a compiled regular expression matching a quoted string.""" return re.compile(r""[^"]*"")
def symbol(): + """Return a compiled regular expression matching a symbol.""" return re.compile(r"\w+")
def count(): + """Return a compiled regular expression matching a decimal + or hexidecimal number.""" return re.compile(r"([1-9][0-9]*|0x[0-9a-fA-F]+)")
def bitdef(): + """Return a list of expressions matching an individual bit slice""" return symbol, ":", count, -1
def _bitdeflist(): + """Return a list of expressions matching a bitfield.""" return bitdef, -1, (",", bitdef)
def bitfield(): + """Return a list of expressions matching a bitfield.""" return -2, _bitdeflist
def array(): + """Return a list of expressions matching an array.""" return symbol, '[', count, ']'
def _typedef(): + """Return a compiled regular expression matching a primitive type.""" return re.compile(r"(%s)" % "|".join(TYPES))
def definition(): + """Return a list of expressions matching a field definition.""" return _typedef, [array, bitfield, symbol], ";"
def seekto(): + """Return an expression matching a seekto directive.""" return keyword("seekto"), count
def seek(): + """Return an expression matching a seek directive.""" return keyword("seek"), count
def printoffset(): + """Return an expression matching a printoffset directive.""" return keyword("printoffset"), string
def directive(): + """Return an expression matching a CHIRP directive.""" return "#", [seekto, seek, printoffset], ";"
def _block_inner(): + """Return an expression matching an inner block.""" return -2, [definition, struct, directive]
def _block(): + """Return an expression matching a block.""" return "{", _block_inner, "}"
def struct_defn(): + """Return an expression matching a structure definition.""" return symbol, _block
def struct_decl(): + """Return an expression matching a structure declaration.""" return [symbol, _block], [array, symbol]
def struct(): + """Return an expression matching a structure definition or declaration.""" return keyword("struct"), [struct_defn, struct_decl], ";"
def _language(): + """Return an expression for the entire language.""" return _block_inner
def parse(data): + """Parse the data using the grammar defined in this file.""" lines = data.split("\n") for index, line in enumerate(lines): if '//' in line: lines[index] = line[:line.index('//')]
- class FakeFileInput: + class FakeFileInput(object): """Simulate line-by-line file reading from @data""" line = -1
def isfirstline(self): + """Return True if reading the first line.""" return self.line == 0
def filename(self): + """Return the filename.""" return "input"
def lineno(self): + """Return the current line number.""" return self.line
def __iter__(self): + """Return an interator.""" return self
def next(self): + """Read the next line.""" self.line += 1 try: # Note, FileInput objects keep the newlines diff --git a/tools/cpep8.lintful b/tools/cpep8.lintful index 68c32b7..caf4dcf 100644 --- a/tools/cpep8.lintful +++ b/tools/cpep8.lintful @@ -2,7 +2,6 @@ # DO NOT ADD NEW FILES!! Instead, fix the code to be compliant. # Over time, this list should shrink and (eventually) be eliminated. ./chirp/bitwise.py -./chirp/bitwise_grammar.py ./chirp/chirp_common.py ./chirp/directory.py ./chirp/drivers/alinco.py

# HG changeset patch # User Zachary T Welch zach@mandolincreekfarm.com # Fake Node ID 9d36a35567b1e336c031255a40c4999d74aa9ecb
Fix pylint issues in bitwise.py (#159)
diff --git a/chirp/bitwise.py b/chirp/bitwise.py index 39efe8f..d70292f 100644 --- a/chirp/bitwise.py +++ b/chirp/bitwise.py @@ -57,6 +57,10 @@ # as integers directly (for int types). Strings and BCD arrays # behave as expected.
+""" +Parse raw data into a python structure defined by a memory layout. +""" + import struct import os import logging @@ -73,19 +77,22 @@ class ParseError(Exception):
def format_binary(nbits, value, pad=8): + """Format some bits in binary @value into a string, padded out to @pad.""" s = "" - for i in range(0, nbits): + for _ in range(0, nbits): s = "%i%s" % (value & 0x01, s) value >>= 1 return "%s%s" % ((pad - len(s)) * ".", s)
def bits_between(start, end): + """Return a bitmask from start to end.""" bits = (1 << (end - start)) - 1 return bits << start
def pp(structure, level=0): + """Pretty print the given @structure, indented with @level spaces.""" for i in structure: if isinstance(i, list): pp(i, level+2) @@ -135,7 +142,8 @@ def set_string(char_array, string): array_copy(char_array, list(string))
-class DataElement: +class DataElement(object): + """Abstract class for encapsulating a piece of data.""" _size = 1
def __init__(self, data, offset, count=1): @@ -144,38 +152,49 @@ class DataElement: self._count = count
def size(self): + """Return the size of this element in bits.""" return self._size * 8
def get_offset(self): + """Return the offset into the data block.""" return self._offset
- def _get_value(self, data): - raise Exception("Not implemented") + def _get_value(self, dummy): + """Return the value, implemented by subclass.""" + raise Exception("Not implemented for %s" % self.__class__)
def get_value(self): + """Return the parsed value.""" value = self._data[self._offset:self._offset + self._size] return self._get_value(value)
- def set_value(self, value): + def set_value(self, dummy): + """Set the value, implemented by subclass.""" raise Exception("Not implemented for %s" % self.__class__)
def get_raw(self): + """Return the raw binary value.""" return self._data[self._offset:self._offset+self._size]
def set_raw(self, data): + """Set the raw binary value.""" self._data[self._offset] = data[:self._size]
def __getattr__(self, name): + """Return the named attribute, implemented by subclass.""" raise AttributeError("Unknown attribute %s in %s" % (name, self.__class__))
def __repr__(self): + """Return a printable representation of this element.""" return "(%s:%i bytes @ %04x)" % (self.__class__.__name__, self._size, self._offset)
class arrayDataElement(DataElement): + """Encapsulate array element.""" + def __repr__(self): if isinstance(self.__items[0], bcdDataElement): return "%i:[(%i)]" % (len(self.__items), int(self)) @@ -189,16 +208,19 @@ class arrayDataElement(DataElement): return s
def __init__(self, offset): + super(self.__class__, self).__init__(None, offset, 0) self.__items = [] - self._offset = offset
def append(self, item): + """Append a data element to the array.""" self.__items.append(item)
def get_value(self): + """Return the list of data elements.""" return list(self.__items)
def get_raw(self): + """Return the raw binary value of the array.""" return "".join([item.get_raw() for item in self.__items])
def __setitem__(self, index, val): @@ -263,6 +285,7 @@ class arrayDataElement(DataElement): self.__items[i].set_value(value[i])
def index(self, value): + """Return the index of the first data element with the given @value.""" index = 0 for i in self.__items: if i.get_value() == value: @@ -274,6 +297,7 @@ class arrayDataElement(DataElement): return iter(self.__items)
def items(self): + """Generate list of items.""" index = 0 for item in self.__items: yield (str(index), item) @@ -287,6 +311,7 @@ class arrayDataElement(DataElement):
class intDataElement(DataElement): + """Represent a integer data element.""" def __repr__(self): fmt = "0x%%0%iX" % (self._size * 2) return fmt % int(self) @@ -415,6 +440,7 @@ class intDataElement(DataElement):
class u8DataElement(intDataElement): + """Represent a 1-byte unsigned integer.""" _size = 1
def _get_value(self, data): @@ -425,6 +451,7 @@ class u8DataElement(intDataElement):
class u16DataElement(intDataElement): + """Represent a 2-byte big-endian unsigned integer.""" _size = 2 _endianess = ">"
@@ -437,10 +464,12 @@ class u16DataElement(intDataElement):
class ul16DataElement(u16DataElement): + """Represent a 2-byte little-endian unsigned integer.""" _endianess = "<"
class u24DataElement(intDataElement): + """Represent a 3-byte big-endian unsigned integer.""" _size = 3 _endianess = ">"
@@ -461,10 +490,12 @@ class u24DataElement(intDataElement):
class ul24DataElement(u24DataElement): + """Represent a 3-byte little-endian unsigned integer.""" _endianess = "<"
class u32DataElement(intDataElement): + """Represent a 4-byte big-endian unsigned integer.""" _size = 4 _endianess = ">"
@@ -477,10 +508,12 @@ class u32DataElement(intDataElement):
class ul32DataElement(u32DataElement): + """Represent a 4-byte little-endian unsigned integer.""" _endianess = "<"
class i8DataElement(u8DataElement): + """Represent a 1-byte signed integer.""" _size = 1
def _get_value(self, data): @@ -491,6 +524,7 @@ class i8DataElement(u8DataElement):
class i16DataElement(intDataElement): + """Represent a 2-byte big-endian signed integer.""" _size = 2 _endianess = ">"
@@ -503,10 +537,12 @@ class i16DataElement(intDataElement):
class il16DataElement(i16DataElement): + """Represent a 2-byte little-endian signed integer.""" _endianess = "<"
class i24DataElement(intDataElement): + """Represent a 3-byte big-endian signed integer.""" _size = 3 _endianess = ">"
@@ -527,10 +563,12 @@ class i24DataElement(intDataElement):
class il24DataElement(i24DataElement): + """Represent a 3-byte little-endian signed integer.""" _endianess = "<"
class i32DataElement(intDataElement): + """Represent a 4-byte big-endian signed integer.""" _size = 4 _endianess = ">"
@@ -543,10 +581,12 @@ class i32DataElement(intDataElement):
class il32DataElement(i32DataElement): + """Represent a 4-byte little-endian signed integer.""" _endianess = "<"
class charDataElement(DataElement): + """Represent a character.""" _size = 1
def __str__(self): @@ -563,17 +603,21 @@ class charDataElement(DataElement):
class bcdDataElement(DataElement): + """Represent a binary-coded decimal data element.""" def __int__(self): tens, ones = self.get_value() return (tens * 10) + ones
def set_bits(self, mask): + """Set the bits specified in the given @mask.""" self._data[self._offset] = ord(self._data[self._offset]) | int(mask)
def clr_bits(self, mask): + """Clear the bits specified in the given @mask.""" self._data[self._offset] = ord(self._data[self._offset]) & ~int(mask)
def get_bits(self, mask): + """Retrieve the bits specified in the given @mask.""" return ord(self._data[self._offset]) & int(mask)
def set_raw(self, data): @@ -595,14 +639,17 @@ class bcdDataElement(DataElement):
class lbcdDataElement(bcdDataElement): + """Represent a little-endian binary-coded decimal data element.""" _size = 1
class bbcdDataElement(bcdDataElement): + """Represent a big-endian binary-coded decimal data element.""" _size = 1
class bitDataElement(intDataElement): + """Represent a bitwise data element.""" _nbits = 0 _shift = 0 _subgen = u8DataElement # Default to a byte @@ -632,6 +679,7 @@ class bitDataElement(intDataElement):
class structDataElement(DataElement): + """Represent a structure.""" def __repr__(self): s = "struct {" + os.linesep for prop in self._keys: @@ -695,13 +743,11 @@ class structDataElement(DataElement):
def size(self): size = 0 - for name, gen in self._generators.items(): + for _, gen in self._generators.items(): if not isinstance(gen, list): gen = [gen]
- i = 0 for el in gen: - i += 1 size += el.size() return size
@@ -709,21 +755,23 @@ class structDataElement(DataElement): size = self.size() / 8 return self._data[self._offset:self._offset+size]
- def set_raw(self, buffer): - if len(buffer) != (self.size() / 8): + def set_raw(self, buf): + if len(buf) != (self.size() / 8): raise ValueError("Struct size mismatch during set_raw()") - self._data[self._offset] = buffer + self._data[self._offset] = buf
def __iter__(self): for item in self._generators.values(): yield item
def items(self): + """Generate the list of items.""" for key in self._keys: yield key, self._generators[key]
-class Processor: +class Processor(object): + """Parse some data according to a memory layout."""
_types = { "u8": u8DataElement, @@ -749,14 +797,17 @@ class Processor: self._offset = offset self._obj = None self._user_types = {} + self._generators = None
def do_symbol(self, symdef, gen): + """Parse a symbol.""" name = symdef[1] self._generators[name] = gen
def do_bitfield(self, dtype, bitfield): - bytes = self._types[dtype](self._data, 0).size() / 8 - bitsleft = bytes * 8 + """Parse a bitfield.""" + fbytes = self._types[dtype](self._data, 0).size() / 8 + bitsleft = fbytes * 8
for _bitdef, defn in bitfield: name = defn[0][1] @@ -765,6 +816,7 @@ class Processor: raise ParseError("Invalid bitfield spec")
class bitDE(bitDataElement): + """A bitwise data element, @bits wide located at @bitsleft.""" _nbits = bits _shift = bitsleft _subgen = self._types[dtype] @@ -773,22 +825,25 @@ class Processor: bitsleft -= bits
if bitsleft: - LOG.warn("WARNING: %i trailing bits unaccounted for in %s" % - (bitsleft, bitfield)) + LOG.warn("WARNING: %i trailing bits unaccounted for in %s", + bitsleft, bitfield)
- return bytes + return fbytes
def do_bitarray(self, i, count): + """Parse a bitarray.""" if count % 8 != 0: raise ValueError("bit array must be divisible by 8.")
class bitDE(bitDataElement): + """A bitwise data element, 1 bit wide.""" _nbits = 1 _shift = 8 - i % 8
return bitDE(self._data, self._offset)
def parse_defn(self, defn): + """Parse a definition.""" dtype = defn[0]
if defn[1][0] == "bitfield": @@ -821,6 +876,7 @@ class Processor: self._generators[name] = res
def parse_struct_decl(self, struct): + """Parse a structure declaration.""" block = struct[:-1] if block[0][0] == "symbol": # This is a pre-defined struct @@ -834,7 +890,7 @@ class Processor: count = 1
result = arrayDataElement(self._offset) - for i in range(0, count): + for _ in range(0, count): element = structDataElement(self._data, self._offset, count, name=name) result.append(element) @@ -849,11 +905,13 @@ class Processor: self._generators[name] = result
def parse_struct_defn(self, struct): + """Parse a structure definition.""" name = struct[0][1] block = struct[1:] self._user_types[name] = block
def parse_struct(self, struct): + """Parse a structure.""" if struct[0][0] == "struct_defn": return self.parse_struct_defn(struct[0][1]) elif struct[0][0] == "struct_decl": @@ -862,6 +920,7 @@ class Processor: raise Exception("Internal error: What is `%s'?" % struct[0][0])
def parse_directive(self, directive): + """Parse a directive.""" name = directive[0][0] value = directive[0][1][0][1] if name == "seekto": @@ -869,10 +928,11 @@ class Processor: elif name == "seek": self._offset += int(value, 0) elif name == "printoffset": - LOG.debug("%s: %i (0x%08X)" % - (value[1:-1], self._offset, self._offset)) + LOG.debug("%s: %i (0x%08X)", + value[1:-1], self._offset, self._offset)
def parse_block(self, lang): + """Parse a block.""" for t, d in lang: if t == "struct": self.parse_struct(d) @@ -882,12 +942,14 @@ class Processor: self.parse_directive(d)
def parse(self, lang): + """Parse the data using the specified @lang.""" self._generators = structDataElement(self._data, self._offset) self.parse_block(lang) return self._generators
def parse(spec, data, offset=0): + """Parse the @data according to @spec, starting at @offset.""" ast = bitwise_grammar.parse(spec) p = Processor(data, offset) return p.parse(ast) diff --git a/tools/cpep8.lintful b/tools/cpep8.lintful index caf4dcf..3178dae 100644 --- a/tools/cpep8.lintful +++ b/tools/cpep8.lintful @@ -1,7 +1,6 @@ # cpep8.lintful: The list of files that do not meet pylint standards. # DO NOT ADD NEW FILES!! Instead, fix the code to be compliant. # Over time, this list should shrink and (eventually) be eliminated. -./chirp/bitwise.py ./chirp/chirp_common.py ./chirp/directory.py ./chirp/drivers/alinco.py
participants (5)
-
Dan Smith
-
Jim Unroe
-
Tom Hayward
-
Zach Welch
-
Zachary T Welch