-
-
Notifications
You must be signed in to change notification settings - Fork 383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PICARD-558: Initial port to Py3 #664
Conversation
8dae232
to
c1b6543
Compare
picard/ui/coverartbox.py
Outdated
if url.scheme() in ('https', 'http'): | ||
path = url.encodedPath() | ||
|
||
if url.scheme() == 'http': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous code was url.scheme() in ('https', 'http'):
, is this change intentionnal ?
picard/ui/infodialog.py
Outdated
@@ -297,7 +297,7 @@ def _display_info_tab(self): | |||
if album.errors: | |||
tabWidget.setTabText(tab_index, _("&Errors")) | |||
text = '<br />'.join(map(lambda s: '<font color="darkred">%s</font>' % | |||
'<br />'.join(unicode(QtCore.Qt.escape(s)) | |||
'<br />'.join(str(cgi.escape(s)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://wiki.qt.io/Transition_from_Qt_4.x_to_Qt5 says Qt.escape()
is deprecated in favor of QString.toHtmlEscaped()
, cgi.escape()
looks weird here, and it doesn't escape "
by default.
There is also Py3 html.escape()
https://docs.python.org/3/library/html.html
https://wiki.python.org/moin/EscapingHtml
Not sure which one is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QString.toHtmlEscaped() in doesn't work python it's a C only method. We can use html.escape however.
c9b17f2
to
8905f2e
Compare
picard/cluster.py
Outdated
@@ -464,23 +464,23 @@ def cluster(self, threshold): | |||
# Keep the matches sorted in a heap | |||
heap = [] | |||
|
|||
for y in xrange(self.clusterDict.getSize()): | |||
for x in xrange(y): | |||
for y in rane(self.clusterDict.getSize()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rane()
??
picard/metadata.py
Outdated
@@ -267,7 +267,7 @@ def set(self, name, values): | |||
def __setitem__(self, name, values): | |||
if not isinstance(values, list): | |||
values = [values] | |||
values = filter(None, map(unicode, values)) | |||
values = [value for value in map(str, values) if value is not None] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[str(value) for value in values if value is not None]
?
picard/script.py
Outdated
@@ -425,7 +426,8 @@ def func_unset(parser, name): | |||
# Allow wild-card unset for certain keys | |||
if name in ('performer:*', 'comment:*', 'lyrics:*'): | |||
name = name[:-1] | |||
for key in parser.context.keys(): | |||
key_list = list(parser.context.keys()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for key in parser.context:
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't do that since we are modifying parser.context in the loop. Storing it in an initial list just to be safe.
1ebf223
to
db7c8e0
Compare
I didn't read next lines... ;) |
44fa112
to
4b34890
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of webbrowser2
specific comments.
picard/util/webbrowser2.py
Outdated
platform. Python 2.5 already has *some* support for this, but it's not | ||
enough, in my opinion. See also: | ||
http://sourceforge.net/tracker/index.php?func=detail&aid=1681228&group_id=5470&atid=105470 | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace docstring instead of removing it. It's hard enough to get documentation into the codebase, so let's not remove what we have without at least putting new documentation in place.
@@ -23,65 +23,6 @@ | |||
from PyQt5 import QtWidgets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't comment on code above this line. :( But I wanted to note that with the removed code, os
and sys
are no longer used, so they also no longer need to be import
ed here.
df27879
to
ef66b27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently crashes for me:
Traceback (most recent call last):
File "tagger.py", line 10, in
main(localedir, True)
File "./picard/tagger.py", line 766, in main
tagger = Tagger(picard_args, unparsed_args, localedir, autoupdate)
File "./picard/tagger.py", line 178, in init
setup_gettext(localedir, config.setting["ui_language"], log.debug)
File "./picard/i18n.py", line 76, in setup_gettext
_ugettext_countries = trans_countries.ugettext
AttributeError: 'GNUTranslations' object has no attribute 'ugettext'
@@ -125,8 +125,8 @@ def __init__(self, parent=None): | |||
self.ui.setupUi(self) | |||
self.ui.ui_language.addItem(_('System default'), '') | |||
language_list = [(l[0], l[1], _(l[2])) for l in UI_LANGUAGES] | |||
for lang_code, native, translation in sorted(language_list, key=operator.itemgetter(2), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the operator
import unnecessary, afaict.
for lang_code, native, translation in sorted(language_list, key=operator.itemgetter(2), | ||
cmp=locale.strcoll): | ||
fcmp = lambda x: locale.strxfrm(x[2]) | ||
for lang_code, native, translation in sorted(language_list, key=fcmp): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I found the not-lambda version easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
py3 deprecates cmp, thus the problem.
picard/ui/options/interface.py
Outdated
@@ -223,7 +223,7 @@ def _all_list_items(self): | |||
|
|||
def _added_actions(self): | |||
actions = self._all_list_items() | |||
actions = filter(lambda x: x != 'separator', actions) | |||
actions = [action for action in actions if action != 'separator'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, you can just do actions = set(action for .... )
here.
@@ -33,10 +34,6 @@ | |||
from picard.ui.ui_options_plugins import Ui_PluginsOptionsPage | |||
|
|||
|
|||
def cmp_plugins(a, b): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benefit of the explicitly defined function is that there's only one place it's defined, instead of twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Py3 deprecates cmp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @mineo. If we're making the same function twice, it would be better to make it once.
fcmp = lambda x: locale.strxfrm(x[2])
is exactly the same as
def fcmp(x):
return locale.strxfrm(x[2])
And can be used in the same manner, but the latter has the benefit of being unittestable and docstringable.
(void)Py_InitModule("astrcmp", AstrcmpMethods); | ||
} | ||
PyModuleDef_HEAD_INIT, | ||
"astrcmp", /* name of module */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: either let all comments here begin one space after the comma or align the slashes.
picard/util/imageinfo.py
Outdated
import struct | ||
from picard.compat import StringIO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're moving to Python 3, just remove compat.py
and use whatever the correct import is.
This is not a working port, I am currently working on it. It wont run and it doesn't pass the tests. |
sip.setapi is no longer needed with PyQt5 and py3. Thus it was removed.
Tested with py3.5 and pyqt5.8, reported issues were all fixed. Good job, @samj1912 ! |
5441854
to
72ac5b3
Compare
https://tickets.metabrainz.org/browse/PICARD-588