readable validation with python regex and simple logic

Question

I have come with such a code for checking IP addresses. But would like to know how clean and good it is from 1 to 10. What I care about are readability and simplicity. Please give me any feedback.

Should I go here with unittests?

import re
import logging

logging.basicConfig(level=logging.ERROR)

pattern = re.compile(r'^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$')

def is_in_bounds(s): 
    return int(s) >= 0 and int(s) <= 255

def is_leading_zeros(s):
    if s is not '0':
        return s[0] is not '0'
    return True

def validate_ipv4(ipv4: str, expectFail: bool = False) -> bool:
    logging.info(f"Validating IP '{ipv4}' START.")

    try:
        match = pattern.match(ipv4)
        assert(match)

        groups = match.groups()
        assert(len(groups)==4)

        for s in match.groups():
            assert(is_leading_zeros(s))
            assert(is_in_bounds(s))
    
        logging.info(f"Validating IP '{ipv4}' SUCCESS.")

    except AssertionError:
        logging.info(f"Validating IP '{ipv4}' FAILED.", exc_info=True)
        if not expectFail:
            raise


if __name__ == '__main__':
    octets = []
    octets.extend(range(0,3))
    octets.extend(range(9,12))
    octets.extend(range(98,102))
    octets.extend(range(198,202))
    octets.extend(range(250,256))

    for i in octets:
        for j in octets:
            for k in octets:
                for l in octets:
                    validate_ipv4(f'{i}.{j}.{k}.{l}')

    octets = []
    octets.extend(range(-3,0))
    octets.extend(range(256, 260))  

    exceptions = []

    for i in octets:
        for j in octets:
            for k in octets:
                for l in octets:
                    validate_ipv4(f'{i}.{j}.{k}.{l}', True)

Answer 1

When to use is not

Your usage of is not is incorrect. You want to use == when you're testing equality, like when you check if s doesn't equal a '0'. You want to use is not when checking for identity, like when you're ensuring two variables are of the same object.

def is_leading_zeros(s: str) -> bool:
    if s != '0':
        return s[0] != '0'
    return True

Clearer comparisons

Python has a syntax that allows you to do this:

def is_in_bounds(s: str) -> int:
    return 0 <= int(s) <= 255

Type Hints

You should add types hints to your functions, which display what types are accepted and returned from your functions. See above for an example of this.

Answer 2

As Linny noted, is_leading_zeros has an error because you want != intead of is not. My Python 3.8.2 actually says SyntaxWarning: "is not" with a literal. Did you mean "!="? on those two lines. But my bigger problem is with the name of the function: is_leading_zeros(s) is True when s doesn't have leading zeros (a docstring could have cleared that up). You use it correctly in your code, but its name is the opposite of what you claim. You could call it has_no_leading_zeros, but that could lead to not has_no_leading_zeros(s) in other code, and I prefer to avoid double negatives. I would call it has_leading_zeros, and negate its use. Also, I would prefer s.startswith('0') to s[0]=='0'.

For the same reason of avoiding double negatives, I'm going to change expectFail to expectPass.

Linny also noted that you can simplify the is_in_bounds comparison and that you are missing some type hints. I've kept those changes as well.

You log that the validation failed when you expected it to fail. This would confuse me if I were looking through my logs and just saw "FAILED". I'll tack on "as expected".

Presumably, you want another python script to be able to use your function to test if something is a valid ipv4 address. But in that case, I would not expect a validator to raise an AssertionError. Let's separate out the logging and assertion portions. I'll also change for s in match.groups() into a generator expression and use all in order to shorten things up. That code feels more readable to me. Unfortunately, this loses the ability to have assertions on each line of the validation.

Are you familiar with itertools? You can change out your nested for loop with
for i,j,k,l in itertools.product(octets,repeat=4).

pattern is too generic of a name. Let's go with ipv4pattern.

Instead of ipv4pattern.match(ipv4), we can use ipv4pattern.fullmatch(ipv4) and drop the ^ and $ from the pattern. It's more readable that way. We could also change the pattern to the shorter r'\.'.join([r'(\d{1,3})']*4), but I don't find that as readable, so I won't do that.

You take several lines to build your octets list. I would go with list(range(0,3))+list(range(9,12))+..., but break it into two to avoid too long of a line.

You never use exceptions = [].

Your unit testing focuses completely on is_in_bounds. You never test that leading zeros aren't allowed, you never test that there are only four groups, and you never test that the pattern match succeeded. You also don't test that the validation fails if one of the groups is out of range, but the others are ok.

The final code:

import re
import logging
import itertools

logging.basicConfig(level=logging.ERROR)

ipv4pattern = re.compile(r'(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})')

def is_in_bounds(s:str) -> bool:
    return 0 <= int(s) <= 255

def has_leading_zeros(s:str) -> bool:
    return s != '0' and s.startswith('0')

def is_valid_ipv4(ipv4:str) -> bool:
    match = ipv4pattern.fullmatch(ipv4)
    if not match:
        return False
    groups = match.groups()
    if len(groups)!=4:
        return False
    return all( ( is_in_bounds(s) and not has_leading_zeros(s)
                        for s in match.groups() ) )

def validate_ipv4(ipv4: str, expectPass: bool = True) -> bool:
    logging.info(f"Validating IP '{ipv4}' START.")
    isvalid = is_valid_ipv4(ipv4)
    assert(isvalid == expectPass)
    if expectPass:
        logging.info(f"Validating IP '{ipv4}' SUCCESS.")
    else:
        logging.info(f"Validating IP '{ipv4}' FAILED, as expected.")

if __name__ == '__main__':
    octets =  list(range(  0,  3))+list(range(  9, 12))+list(range( 98,102))
    octets += list(range(198,202))+list(range(250,256))

    for i,j,k,l in itertools.product(octets,repeat=4):
        validate_ipv4(f'{i}.{j}.{k}.{l}')

    octets = list(range(-3,0))+list(range(256, 260))

    for i,j,k,l in itertools.product(octets,repeat=4):
        validate_ipv4(f'{i}.{j}.{k}.{l}', False)