start of work to add linting

Signed-off-by: Vanessa Sochat <[email protected]>

.travis.yml

@@ -9,6 +9,14 @@ python:
   - "2.7"
   - "3.5"
 
+matrix:
+  include:
+  - name: "Python 3.5"
+    python: "3.5"
+  - name: "Python 2.7"
+    python: "2.7"
+    env: RUN_PYLINT=no
+
 install:
   - pip install pylint pydicom
   - cd $TRAVIS_BUILD_DIR/
@@ -17,4 +25,5 @@ install:
   - pylint --version
 
 script:
+  - if [ -z "${RUN_PYLINT}" ]; then pylint deid; fi;
   - python -m unittest discover -s $TRAVIS_BUILD_DIR/deid/tests/ -p '[t|T]est*.py'

CHANGELOG.md

@@ -14,6 +14,7 @@ and **Merged pull requests**. Critical items to know are:
 Referenced versions in headers are tagged on Github, in parentheses are for pypi.
 
 ## [vxx](https://github.com/pydicom/deid/tree/master) (master)
+ - adding pylint to clean up code (0.1.32)
  - removing dependency that isn't used (simplejson) (0.1.31)
  - updating cleaner to use pixel array (0.1.30)
  - validation function should use debug verbository, bumping license year [#92](https://github.com/pydicom/deid/issues/92) (0.1.29)

MANIFEST.in

@@ -5,5 +5,4 @@ global-exclude *.pyc
 prune __pycache__
 prune *.pyc
 prune deid/tests
-prune deid/app
 prune *OLD

deid/config/__init__.py

@@ -164,7 +164,7 @@ def _init_deid(self, deid=None, base=False, default_base='dicom'):
         if deid is None:
             deid = []
 
-        if not isinstance(deid,list):
+        if not isinstance(deid, list):
             deid = [deid]
 
         if base is True:

deid/config/standards.py

@@ -26,7 +26,7 @@
 formats = ['dicom']
 
 # Supported Sections
-sections = ['header','labels','filter']
+sections = ['header', 'labels', 'filter']
 
 actions = ('ADD',
            'BLANK',

deid/config/utils.py

@@ -25,6 +25,8 @@
 
 '''
 
+# pylint: skip-file
+
 from deid.logger import bot
 from deid.utils import read_file
 from deid.data import data_base
@@ -51,8 +53,7 @@ def load_combined_deid(deids):
 
     '''
     if not isinstance(deids,list):
-        bot.warning("load_combined_deids expects a list.")
-        sys.exit(1)
+        bot.exit("load_combined_deids expects a list.")
 
     found_format = None
     deid = None
@@ -72,9 +73,8 @@ def load_combined_deid(deids):
                 found_format = next_deid['format']
             else:
                 if found_format != next_deid['format']:
-                    bot.error('Mismatch in deid formats, %s and %s' %(found_format,
-                                                                      next_deid['format']))
-                    sys.exit(1)
+                    bot.exis('Mismatch in deid formats, %s and %s' %(found_format,
+                                                                     next_deid['format']))
 
             # If it's the first one, use as starter template
             if deid is None:
@@ -99,7 +99,7 @@ def load_combined_deid(deids):
                         deid['header'] = deid['header'] + next_deid['header']
 
         else:
-            bot.warning('Problem loading %s, skipping.' %single_deid)
+            bot.warning('Problem loading %s, skipping.' % single_deid)
     return deid
 
 
@@ -148,8 +148,8 @@ def load_deid(path=None):
         elif bool(re.match('format', line, re.I)):
             fmt = re.sub('FORMAT|(\s+)','',line).lower()
             if fmt not in formats:
-                bot.error("%s is not a valid format." %fmt)
-                sys.exit(1)
+                bot.exit("%s is not a valid format." %fmt)
+
             # Set format
             config['format'] = fmt
             bot.debug("FORMAT set to %s" %fmt)
@@ -167,8 +167,8 @@ def load_deid(path=None):
                 section_name = ' '.join(parts[1:])          
             section = re.sub('[%]|(\s+)','',parts[0]).lower()
             if section not in sections:
-                bot.error("%s is not a valid section." %section)
-                sys.exit(1)
+                bot.exit("%s is not a valid section." % section)
+
             config = add_section(config=config,
                                  section=section,
                                  section_name=section_name)
@@ -228,8 +228,7 @@ def find_deid(path=None):
                       if x.startswith('deid')]
 
         if len(contenders) == 0:
-            bot.error("No deid settings files found in %s, exiting." %(path))
-            sys.exit(1)
+            bot.exit("No deid settings files found in %s, exiting." %(path))
 
         elif len(contenders) > 1:   
             bot.warning("Multiple deid files found in %s, will use first." %(path))
@@ -239,8 +238,7 @@ def find_deid(path=None):
 
     # We have a file path at this point
     if not os.path.exists(path):
-        bot.error("Cannot find deid file %s, exiting." %(path))
-        sys.exit(1)
+        bot.exit("Cannot find deid file %s, exiting." %(path))
 
     return path
 
@@ -352,27 +350,25 @@ def parse_member(members, operator=None):
             try:
                 field, value = member.split(' ',1)
             except ValueError:
-                bot.error('%s for line %s must have field and values, exiting.' %(action,member))
-                sys.exit(1)
+                bot.exit('%s for line %s must have field and values, exiting.' %(action,member))
 
         # Missing, empty, expect only a field
         elif action in ['missing', 'empty', 'present']:
             field = member.strip()
         else:
-            bot.error('%s is not a valid filter action.' %action)
-            sys.exit(1)
+            bot.exit('%s is not a valid filter action.' %action)
 
         actions.append(action)
         fields.append(field.strip())
 
         if value is not None:
             values.append(value.strip())  
 
-    entry = {'action':actions,
-             'field':fields,
+    entry = {'action': actions,
+             'field': fields,
              'operator': main_operator,
-             'InnerOperators':operators,
-             'value': values }
+             'InnerOperators': operators,
+             'value': values}
     return entry
 
 
@@ -389,16 +385,13 @@ def add_section(config,section,section_name=None):
     '''
 
     if section is None:
-        bot.error('You must define a section (e.g. %header) before any action.')
-        sys.exit(1)
+        bot.exit('You must define a section (e.g. %header) before any action.')
 
     if section == 'filter' and section_name is None:
-        bot.error("You must provide a name for a filter section.")
-        sys.exit(1)
+        bot.exit("You must provide a name for a filter section.")
 
     if section not in sections:
-        bot.error("%s is not a valid section." %section)
-        sys.exit(1)
+        bot.exit("%s is not a valid section." %section)
 
     if section not in config:
 
@@ -434,37 +427,35 @@ def parse_action(section, line, config, section_name=None):
     '''
 
     if not line.upper().startswith(actions):
-        bot.error("%s is not a valid action line." %line)
-        sys.exit(1)
+        bot.exit("%s is not a valid action line." % line)
 
     # We may have to deal with cases of spaces
     parts = line.split(' ')
     action = parts.pop(0).replace(' ','')
 
     # What field is the action for?
     if len(parts) < 1:
-        bot.error("%s requires a FIELD value, but not found." %(action))        
-        sys.exit(1)
+        bot.exit("%s requires a FIELD value, but not found." % action) 
 
     field = parts.pop(0)
 
     # Actions that require a value
     if action in [ "ADD", "REPLACE", "JITTER" ]:
         if len(parts) == 0:
-            bot.error("%s requires a VALUE, but not found" %(action))        
-            sys.exit(1)
+            bot.exit("%s requires a VALUE, but not found" % action) 
+
         value = ' '.join(parts[0:])  # get remained of line
         value = value.split('#')[0]  # remove comments
         bot.debug("Adding %s" %line) #
-        config[section].append({ "action":action,
-                                 "field":field,
-                                 "value":value })
+        config[section].append({"action":action,
+                                "field":field,
+                                "value":value})
 
     # Actions that don't require a value
-    elif action in [ "BLANK", "KEEP", "REMOVE" ]:
+    elif action in ["BLANK", "KEEP", "REMOVE"]:
         bot.debug("%s: adding %s" %(section,line))
         config[section].append({ "action":action,
-                                 "field":field })
+                                 "field":field})
 
     return config
 

deid/dicom/actions.py

@@ -22,26 +22,24 @@
 
 '''
 
+from deid.logger import bot
 from deid.config.standards import (
     actions as valid_actions
 )
 
 from .fields import expand_field_expression
 
-from deid.logger import bot
-from pydicom import read_file
-from pydicom._dicom_dict import DicomDictionary
 from deid.utils import (
-    recursive_find,
     get_timestamp,
     parse_value
 )
-from .tags import *
-import tempfile
-import os
-import re
-import sys
-import datetime
+
+from .tags import (
+    add_tag,
+    update_tag,
+    blank_tag,
+    remove_tag
+)
 
 
 # Actions
@@ -94,12 +92,12 @@ def _perform_action(dicom, field, action, value=None, item=None):
                        Defaulting to blanked.''' %(action,
                                                ".".join(valid_actions)))
         action = "BLANK"
-
+       
     if field in dicom and action != "ADD":
 
         # Blank the value
         if action == "BLANK":
-            dicom = blank_tag(dicom,field)
+            dicom = blank_tag(dicom, field)
 
         # Code the value with something in the response
         elif action == "REPLACE":
@@ -156,34 +154,30 @@ def jitter_timestamp(dicom, field, value):
     if not isinstance(value, int):
         value = int(value)
 
-    original = dicom.get(field,None)
+    original = dicom.get(field, None)
 
     if original is not None:
         dcmvr = dicom.data_element(field).VR
-        '''
-        DICOM Value Representation can be either DA (Date) DT (Timestamp),
-        or something else, which is not supported.
-        '''
-        if (dcmvr == 'DA'):
-            '''
-            NEMA-compliant format for DICOM date is YYYYMMDD
-            '''
+
+        # DICOM Value Representation can be either DA (Date) DT (Timestamp),
+        # or something else, which is not supported.
+
+        if dcmvr == 'DA':
+            # NEMA-compliant format for DICOM date is YYYYMMDD
             new_value = get_timestamp(original, jitter_days=value, 
                                       format='%Y%m%d')
 
-        elif (dcmvr == 'DT'):
-            '''
-            NEMA-compliant format for DICOM timestamp is
-            YYYYMMDDHHMMSS.FFFFFF&ZZXX
-            '''
+        elif dcmvr == 'DT':
+            # NEMA-compliant format for DICOM timestamp is
+            # YYYYMMDDHHMMSS.FFFFFF&ZZXX
             new_value = get_timestamp(original, jitter_days=value,
                                       format='%Y%m%d%H%M%S.%f%z')
         else:
             # Do nothing and issue a warning.
             new_value = None
             bot.warning("JITTER not supported for %s with VR=%s" % (field, 
                                                                     dcmvr))
-        if (new_value is not None and new_value != original):
+        if new_value is not None and new_value != original:
             # Only update if there's something to update AND there's been change
             dicom = update_tag(dicom,
                                field=field,