TDE
/
amarok
6
0
Fork 0
Code Issues 7 Pull Requests 1 Releases Wiki Activity
Labels Milestones

Drop python2 support. #35

Merged
SlavekB merged 1 commits from feat/drop-python2 into master 1 year ago
Conversation 0 Commits 1 Files Changed 11
SlavekB commented 1 year ago
Owner

Update for PyTQt.

Although my experience with Python is small, I hope that conversions using 2to3 and subsequent adjustments of the code are fine.

Update for PyTQt. Although my experience with Python is small, I hope that conversions using 2to3 and subsequent adjustments of the code are fine.
SlavekB added 1 commit 1 year ago
8e7e2f5893
Drop python2 support.
SlavekB added this to the R14.1.0 release milestone 1 year ago
SlavekB requested review from MicheleC 1 year ago
MicheleC requested changes 1 year ago
MicheleC left a comment
Owner

Some corrections required related to string.contains.

Some corrections required related to string.contains.
amarok/src/scripts/alarm/alarm.py Outdated
print("[Alarm Script] Received notification: " + str( string ))
if string.contains( "configure" ):
if "configure" in string:
MicheleC commented 1 year ago
Owner

This seems wrong. string is a TQString object, so the call to contains actually calls the TQString method with the same name through C++ extension. I don't think we should change this line.
It may also be a good idea to rename string to something else, since it could be confused with the string python2 object.

This seems wrong. `string` is a `TQString` object, so the call to `contains` actually calls the TQString method with the same name through C++ extension. I don't think we should change this line. It may also be a good idea to rename `string` to something else, since it could be confused with the `string` python2 object.
amarok/src/scripts/amarok_live/amarok_live.py Outdated
debug( "Received notification: " + str( string ) )
if string.contains( "configure" ):
if "configure" in string:
MicheleC commented 1 year ago
Owner

As before, this line shoud not be changed. Some for subsequent cotains calls in the following few lines.

As before, this line shoud not be changed. Some for subsequent `cotains` calls in the following few lines.
amarok/src/scripts/templates/python_qt_template.py Outdated
debug( "Received notification: " + str( string ) )
if string.contains( "configure" ):
if "configure" in string:
MicheleC commented 1 year ago
Owner

As per previous comments, replacing the calls to contains seems wrong.

As per previous comments, replacing the calls to `contains` seems wrong.
amarok/src/scripts/webcontrol/WebControl.py Outdated
debug( "Received notification: " + str( string ) )
if string.contains( "configure" ):
if "configure" in string:
MicheleC commented 1 year ago
Owner

As before, replacing contains seems wrong.

As before, replacing `contains` seems wrong.
SlavekB reviewed 1 year ago
amarok/src/scripts/amarok_live/amarok_live.py Outdated
elif string.contains( "customMenuClicked" ):
elif "customMenuClicked" in string:
if "selected" in string:
SlavekB commented 1 year ago
Poster
Owner

Thank you for the comment on string that is TQstring object – I did not realize that in this case contains(...) is the call of the TQString object method and there is no need to change it.

However, you may notice that in the original code the same use of "something" in string has previously been in other places, such as this. It seems that both ways can work. Perhaps it would be good for the same way to be used in all cases.

Thank you for the comment on `string` that is `TQstring` object – I did not realize that in this case `contains(...)` is the call of the TQString object method and there is no need to change it. However, you may notice that in the original code the same use of `"something" in string` has previously been in other places, such as this. It seems that both ways can work. Perhaps it would be good for the same way to be used in all cases.
MicheleC commented 1 year ago
Owner

I did a test to see if things could work or not. It seems that both ways work fine.

# python
>>> from PyTQt.qt import *
>>> string = TQString("Hello world")
>>> string.contains("Hello")
1
>>> string.contains("Hello1")
0
>>> "Hello" in string
True
>>> "Hello1" in string
False
>>> quit()

So we could probably merge the PR as is now.

I did a test to see if things could work or not. It seems that both ways work fine. ``` # python >>> from PyTQt.qt import * >>> string = TQString("Hello world") >>> string.contains("Hello") 1 >>> string.contains("Hello1") 0 >>> "Hello" in string True >>> "Hello1" in string False >>> quit() ``` So we could probably merge the PR as is now.
SlavekB commented 1 year ago
Poster
Owner

PR is already updated to use eventStr.contains(...) in all cases 😉

PR is already updated to use `eventStr.contains(...)` in all cases 😉
MicheleC commented 1 year ago
Owner

Yes, I hadn't noticed the new commit before my previous comment, but just reviewed it now :-)

Yes, I hadn't noticed the new commit before my previous comment, but just reviewed it now :-)
MicheleC marked this conversation as resolved
SlavekB force-pushed feat/drop-python2 from 8e7e2f5893 to 9be5499101 1 year ago
MicheleC approved these changes 1 year ago
MicheleC left a comment
Owner

Looks good.

Looks good.
SlavekB merged commit 9be5499101 into master 1 year ago
SlavekB deleted branch feat/drop-python2 1 year ago

Reviewers

MicheleC approved these changes 1 year ago
The pull request has been merged as 9be5499101.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
MicheleC
Labels
Apply labels
Clear labels
GE/need-info
General - need additional info from contributor PR/keep-branch
Pull request - do not delete branch after merging PR/not-ok
Pull request - need fixing PR/rfc
Pull request - request for comments PR/update-trans
Pull request - update to translation files needed PR/wip
Pull request - work in progress RS/R14.0.x
Related to R14.0.x series RS/R14.1.x
Related to R14.1.x series SL/critical
Severity level - critical SL/major
Severity level - major SL/minor
Severity level - minor SL/normal
Severity level - normal SL/regression
Severity level - regression from previous version SL/trivial
Severity level - trivial SL/wishlist
Severity level - wishlist request ST/duplicate
Status - duplicate of another issue ST/invalid
Status - invalid report ST/notourproblem
Status - not our problem ST/rejected
Status - rejected ST/wontfix
Status - won't fix ST/worksforme
Status - works for me, unable to reproduce
No Label GE/need-info PR/keep-branch PR/not-ok PR/rfc PR/update-trans PR/wip RS/R14.0.x RS/R14.1.x SL/critical SL/major SL/minor SL/normal SL/regression SL/trivial SL/wishlist ST/duplicate ST/invalid ST/notourproblem ST/rejected ST/wontfix ST/worksforme
Milestone
Set milestone
Clear milestone
No items
No Milestone
R14.1.0 release
Assignees
Assign users
Clear assignees
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/amarok#35
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'feat/drop-python2'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes