0

I'm using the argparse module to use create an address object via the command line. But when I feed it invalid arguments (i.e. those that should raise an exception), no exceptions are raised. Worse still, nothing is being logged (unless I create a valid object).

So, this works:

-n Patrick -a "151 Piedmont Ave" -c "Winston Salem" -s "NC" -z 27101

This doesn't:

-l ERROR -n Patrick -a "151 Piedmont Ave" -c "Winston Salem" -s "NC" -z 271

Where am I going wrong?

NOTE: This is homework, so guidance over the absolute answer would be appreciated.

"""
property_address.py
"""
import re
import logging
import argparse

LOG_FILENAME = 'property_address.log'
LOG_FORMAT = "%(asctime)s - %(levelname)s - %(funcName)s - %(message)s"
DEFAULT_LOG_LEVEL = "debug"
LEVELS = {'debug': logging.DEBUG,
          'info': logging.INFO,
          'warning': logging.WARNING,
          'error': logging.ERROR,
          'critical': logging.CRITICAL
         }

def start_logging(filename=LOG_FILENAME, level=None):
    logging.basicConfig(filename=filename, level=level, filemode='w', format=LOG_FORMAT)
    logging.info('Starting up the property address program')

class ZipCodeError(Exception):
    "Custom exception for invalid zip codes."
    pass


class StateError(Exception):
    "Custom exception for invalid state abbreviation."
    pass


class Address(object):
    """
    An address object.
    """

    def __init__(self, name, street_address, city, state, zip_code):
        self._name = name
        self._street_address = street_address
        self._city = city
        self._state = state
        self._zip_code = zip_code
        logging.info('Instantiated an address')

    @property
    def name(self):
        return self._name

    @property
    def street_address(self):
        return self._street_address

    @property
    def city(self):
        return self._city

    @property
    def state(self):
        return self._state

    @state.setter
    def state(self, value):
        "Validate that states are abbreviated as US Postal style."
        state_valid = re.compile(r'[A-Z]{2}$')
        if re.match(state_valid, value):
            self._state = value
        else:
            message = 'STATE Exception: State not in correct format'
            logging.error(message)
            raise StateError()

    @property
    def zip_code(self):
        return self._zip_code

    @zip_code.setter
    def zip_code(self, value):
        "Validate zip codes are five digits."
        zip_valid = re.compile(r'\d{5}$')
        if re.match(zip_valid, value):
            self._zip_code = value
        else:
            message = 'ZIP CODE Exception: Zip code not in correct format'
            logging.error(message)
            raise ZipCodeError()

    def __str__(self):
        return self._name

if __name__ == "__main__":
    parser = argparse.ArgumentParser(description='Set attributes for property address.')
    parser.add_argument(
                        '-l',
                        '--level',
                        dest='level',
                        choices=['DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL'],
                        default='INFO',
                        help='Sets the log level to DEBUG, INFO, WARNING, ERROR, and CRITICAL')
    parser.add_argument(
                        '-n',
                        '--name',
                        dest='name',
                        action='store',
                        required=True,
                        help='Sets the name value of the Address object')
    parser.add_argument(
                        '-a',
                        '--address',
                        dest='address',
                        action='store',
                        required=True,
                        help='Sets the street_address value of the Address object')
    parser.add_argument(
                        '-c',
                        '--city',
                        dest='city',
                        action='store',
                        required=True,
                        help='Sets the city value of the Address object')
    parser.add_argument(
                        '-s',
                        '--state',
                        dest='state',
                        action='store',
                        required=True,
                        help='Sets the state value of the Address object')
    parser.add_argument(
                        '-z',
                        '--zip_code',
                        dest='zip_code',
                        action='store',
                        required=True,
                        help='Sets the zip_code value of the Address object')
    args = parser.parse_args()

    # Start our logger
    start_logging(level=(args.level))

    # Create our Address object
    a = Address(args.name, args.address, args.city, args.state, args.zip_code)
jonrsharpe
  • 115,751
  • 26
  • 228
  • 437
Patrick Beeson
  • 1,667
  • 21
  • 36
  • 1
    Why not test the zip code as part of the argument; see e.g. http://stackoverflow.com/q/25470844/3001761 – jonrsharpe Dec 13 '14 at 12:56
  • That makes sense. But why aren't my exceptions being raised in the first place? – Patrick Beeson Dec 13 '14 at 13:11
  • 1
    Because in `Address.__init__` you assign to `self._zip_code`, bypassing the setter, instead of `self.zip_code`. – jonrsharpe Dec 13 '14 at 13:12
  • Also, you should be more consistent; `zip_code` and `state` can be changed, but the other attributes are read-only. Either put the checking in `__init__` and make them all read-only, or leave the ones without checking as vanilla attributes and ditch their getters. Python makes it easy to put them back later it they turn out to be necessary. – jonrsharpe Dec 13 '14 at 13:19
  • Good point. Looks like a good explanation of properties also exists here: http://chimera.labs.oreilly.com/books/1230000000393/ch08.html#_solution_124 – Patrick Beeson Dec 13 '14 at 13:29
  • Is this a problem with your use of `argparse`, or with your `Address` class? Did you ever look at `args` immediately after `parse_args`? If those matched your expectations (as I suspect they do), then is is not an `argparse problem`! – hpaulj Dec 13 '14 at 23:12

1 Answers1

1

In Address.__init__ you are assigning to e.g. self._zip_code, which bypasses the setter. The minimal fix is therefore to use:

self.zip_code = zip_code
   # ^ note no underscore 

This means the setter is called and your check made.


Beyond that, you have an inconsistency between your attributes; state and zip_code can be changed once set, but the others are read-only. In the likely event that this isn't the desired behaviour, I would remove the getters for attributes without setters and access all directly (i.e. without underscores) in __init__.

Alternatively, if you do want them all to be read-only, remove the setters and put their tests in __init__.


A final option is to test the arguments when parsed, rather than in the class; see e.g. Specify format for input arguments argparse python.

If you still want the class to test its arguments (for when they don't come via argparse), you could consider exposing a @staticmethod for the tests in your class, that then gets called in the setter too:

class Address:

    ...

    @staticmethod
    def valid_zip_code(zip_code, zip_valid=re.compile(r'\d{5}$')):
        if not re.match(zip_valid, zip_code):
            raise ZipCodeError
        return zip_code

    @zip_code.setter
    def zip_code(self, new_zip):
          new_zip = self.valid_zip_code(new_zip)
          self._zip_code = new_zip

...

parser.add_argument('-z', ..., type=Address.valid_zip_code)
Community
  • 1
  • 1
jonrsharpe
  • 115,751
  • 26
  • 228
  • 437