Skip to content

Commit

Permalink
Make exceptions Python 3 compatible
Browse files Browse the repository at this point in the history 
(Thanks to Andy Seit.)
  • Loading branch information
@andreastt
andreastt committed May 23, 2014
1 parent 58a185a commit e68e73e
Showing 1 changed file with 5 additions and 4 deletions.
9 changes: 5 additions & 4 deletions py/selenium/webdriver/firefox/firefox_profile.py
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Copyright 2014 Software Freedom Conservancy
# Copyright 2008-2011 WebDriver committers
# Copyright 2008-2011 Google Inc.
#
Expand Down Expand Up @@ -340,8 +341,8 @@ def get_text(element):
manifest = f.read()
else:
raise IOError('Add-on path is neither an XPI nor a directory: %s' % addon_path)
except (IOError, KeyError), e:
raise AddonFormatError, str(e), sys.exc_info()[2]
except (IOError, KeyError) as e:
raise AddonFormatError(str(e), sys.exc_info()[2])

try:
doc = minidom.parseString(manifest)
Expand All @@ -356,8 +357,8 @@ def get_text(element):
entry = node.nodeName.replace(em, "")
if entry in details.keys():
details.update({entry: get_text(node)})
except Exception, e:
raise AddonFormatError, str(e), sys.exc_info()[2]
except Exception as e:
raise AddonFormatError(str(e), sys.exc_info()[2])

# turn unpack into a true/false value
if isinstance(details['unpack'], basestring):
Expand Down

12 comments on commit e68e73e

@gobuk
Copy link

@gobuk gobuk commented on e68e73e May 25, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should replace 'basestring' with 'str' on line 364 too, for python 3 compatibility

@bayandin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gobuk It can break python 2.x for str (not unicode) because:

try:
    from cStringIO import StringIO as BytesIO
    bytes = str
    str = unicode
except ImportError:
    from io import BytesIO

@andreastt
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no expert in Python 3 compatibility, but I'm quite sure there are more things that needs to be updated. A critical review I think is warranted.

@gobuk
Copy link

@gobuk gobuk commented on e68e73e May 25, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about adding 'basestring = str' after 'from io import BytesIO' on line 33

@pmav99
Copy link

@pmav99 pmav99 commented on e68e73e May 26, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to keep a single codebase for both Python 2 and Python 3, which imho is a quite sensible option if you don't need python <2.6, then you have two options

  • You could insert an extra dependency like six and then use its string_types
  • you could create your own compatibility module where you will define the subset of the six functionality that you need.

If you want to go with the second approach, you could use unio

@dlai0001
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think using a compatibility library would be the best approach.

A while back I suggested using six, and volunteered to refactor that. But the core team thought six was too heavy weight. Perhaps unio which is focused on just the unicode piece wouldn't add too much.

I think the try/catch at import time is a bad approach. It creates unintended side effect with other libraries that does low level string processing such as PIL.

@dlai0001
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also keep in mind, that try/except approach is already also used in other parts of the python bindings code base. Such as, https://github.com/SeleniumHQ/selenium/blob/master/py/selenium/webdriver/remote/webelement.py#L20

so any refactoring to do this probably has to be done all at once. Because they'll all have side effects on one another.

@andreastt
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AutomatedTester, any opinions on this?

@andreastt
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukeis ?

@lukeis
Copy link
Member

@lukeis lukeis commented on e68e73e May 26, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we don't include third party dependencies, but we may in the near future. I don't have much skin in the game. Are there any potential conflict with any other library? If we can't definitively say, then we should probably make our own string utility to use.

@andreastt
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue was also filed about this, which I've now resolved: https://code.google.com/p/selenium/issues/detail?id=7381

@pmav99
Copy link

@pmav99 pmav99 commented on e68e73e May 27, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukeis

Are there any potential conflict with any other library?

I am not sure what you mean. If you use something like this

import sys
PY2 = sys.version_info.major == 2

if PY2:
    pass
else:
    # Python 3 interpreter
    basestring = str

and someone uses a star import (i.e. from selenium.webdriver.firefox.firefox_profile import *) then yes he may have side effects.

That's the reason six and similar compatibility modules usually take this approach

import sys
PY2 = sys.version_info.major == 2

if PY2:
    string_types = (str, unicode)
else:
    string_types = (str,)

my_object = "asdf"
if isinstance(my_object, string_types):
    print("This is a string")

Please sign in to comment.