Is fp closed checks closed first

[novocaine]

for #434

[Lukasa]

This seems reasonable to me. As to @piotr-dobrogost's suggestion, changing the style seems reasonable but I suggest placing it in the except clause rather than the try clause (which I suggest is what Piotr probably meant. =) )

[shazow]

+1, they should either both be hasattr/getattr or both be in a try/except clause.

Also please keep comments and strings consistent with the rest of the codebase (full sentences, start with a capital, end with a period, etc).

Thanks for putting this together. :)

[novocaine]

Amended with suggestions.

I would prefer to have the two checks at the same level of indentation rather than putting the second in the except clause; it reads more like a sequential series of checks, and if there are subsequent checks added later the indentation won't get out of control.

[shazow]

This is good, thank you. I'll merge it later this week. :)

I think in general we prefer hasattr/getattr instead of exception-catching where possible because raising/catching exceptions in a tight loop tends to be rather slow. Also reduces the risk of accidentally catching an unrelated exception. Maybe I'll tweak that later.

[novocaine]

https://twitter.com/raymondh/status/492554332764520448

On Thu, Jul 31, 2014 at 10:38 AM, Andrey Petrov notifications@github.com
wrote:

This is good, thank you. I'll merge it later this week. :)

I think in general we prefer hasattr/getattr instead of exception-catching
where possible because raising/catching exceptions in a tight loop tends to
be rather slow. Also reduces the risk of accidentally catching an unrelated
exception. Maybe I'll tweak that later.

—
Reply to this email directly or view it on GitHub
#435 (comment).

[shazow]

Can't believe I dug this up but...

https://github.com/python-git/python/blob/master/Python/bltinmodule.c#L860

Wonder if C-level exception handling is more efficient than Python-level, though.

In [1]: foo = object()

In [2]: def a():
   ...:     return hasattr(foo, 'bar')
   ...:

In [3]: def b():
   ...:     return getattr(foo, 'bar', None)
   ...:

In [4]: def c():
   ...:     try:
   ...:         return foo.bar
   ...:     except AttributeError:
   ...:         return False
   ...:

In [5]: %timeit a
10000000 loops, best of 3: 38.7 ns per loop

In [6]: %timeit b
10000000 loops, best of 3: 44.6 ns per loop

In [7]: %timeit c
10000000 loops, best of 3: 37.4 ns per loop

¯_(ツ)_/¯

Looks like it doesn't matter much anymore. Will keep it as-is. Thanks @novocaine. :)

[requiredfield]

Just a heads up that this change caused compatibility issues with Python 3 and PyPy for cachecontrol (see psf/cachecontrol#39). In case the urllib3 / requests maintainers / subscribers of this thread want to investigate further!

[sigmavirus24]

@requiredfield we (requests maintainers) do watch urllib3's issues and threads. We also watch cachecontrol. The issue you link to was fixed and the fix was released. It's being included in pip as well. Thank you for thinking of us.

[requiredfield]

Didn't realize you watched cachecontrol already, sorry for the unnecessary ping.

[sigmavirus24]

@requiredfield No need to apologize! I appreciate the ping as I'm sure @shazow does. (@Lukasa was in IRC with the rest of us)