Fix possible ValueError in convert_to_int caused by values like 1px

[yaph]

When trying to parse http://www.wired.com/wiredscience/2013/09/rim-fire-map-color-scale/ a ValueError was raised in convert_to_img, because the page has image width and height values ending in px.

I changed the function to be more liberal regarding dimension values, by extracting the digits before casting to int. I added a test for this.

Not sure though if the value should be converted to int at all or kept as a string.

[coveralls]

Coverage decreased (-2.35%) when pulling ebbd936 on yaph:convertint into a8430c0 on michaelhelmick:master.

[michaelhelmick]

Very good. I'm glad that adding tests and modifying this source code has been understandable.

Now for the pull request, the only gripe I have is that it is invalid HTML to have width="100px" in the img tag. So, should we even parse it?

We could either:

1. Accept this pull request and gracefully accept a width or height that might be invalid
2. Modify the pull request and except both TypeError and ValueError, therefore ignoring the value all together

If we chose the second option, the convert_to_int method would look like this:

def convert_to_int(value):
    """Attempts to convert a specified value to an integer

    :param value: Content to be converted into an integer
    :type value: string or int

    """
    try:
        return int(value)
    except TypeError:
        return None

Thoughts @ashibble?

[ashibble]

Although it may be invalid to write width="100px", I have seen it done before. I feel that the intended meaning is the same for px sizes, and so it wouldn't necessarily be a BAD thing.

I do think that we should ignore sizes with em or % in them, as these may not display correctly converted to px sizes. Changing % to pixels would reflect an incorrect width.

[yaph]

As I mentioned, I'm not sure, whether the value should be changed at all, except maybe for calling strip() on it.

I agree that just removing/ignoring em and % is not a good idea.

That being said valid HTML and real-world usage are 2 different things, so I suggest the following.

    if value is None:
        return None

    value = value.strip().strip('px')
    try:
        return int(value)
    except TypeError:
        return None
    except ValueError:
        return None

If a value contains a px suffix, although not valid, accept it and use the digits only. Everything else that can't be converted to int will be omitted.

[michaelhelmick]

We could probably do this:

pattern = re.compile(r'(?P<value>\d+)?px+')
def convert_to_int(value):
    try:
        match = re.search(pattern, value))
        if match:
            value = int(match.group('value')
    except (TypeError, ValueError):
        return None

I don't know if the double strip or the regex pattern search is faster.
I'll test that tomorrow, I'm getting sleepy and don't feel like testing it right now, haha.

[michaelhelmick]

@yaph

Alright, it appears the basic double strip is faster! Think you can update your pull request?

Except... instead of excepting TypeError and ValueError separately, can you do them in one like like in my last comment? Thanks!

[yaph]

Okay I'll update it soon.

[michaelhelmick]

Thanks!

Sent from my iPhone

On Sep 9, 2013, at 7:35 PM, Ramiro Gómez notifications@github.com wrote:

Okay I'll update it soon.

—
Reply to this email directly or view it on GitHub.

[coveralls]

Coverage decreased (-1.17%) when pulling 7db8a0a on yaph:convertint into a8430c0 on michaelhelmick:master.

[michaelhelmick]

One more thing if it isn't too much trouble!

Change

value = value.strip().strip('px')

to

value = value.strip(' px')

I actually just did some reading and found out that the all instances of characters passed to strip are removed from the string!
http://docs.python.org/2/library/stdtypes.html#str.strip

Crazy, I thought it was just striping the phrase. My change has a space along side p and x value.strip(' px')

[yaph]

Interesting, I wasn't aware of that either, I'll update that.

[coveralls]

Coverage decreased (-1.17%) when pulling f35cbef on yaph:convertint into a8430c0 on michaelhelmick:master.

[michaelhelmick]

Merged, thanks!

[yaph]

You're welcome! Glad I could contribute to lassie.