-
Notifications
You must be signed in to change notification settings - Fork 12
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
Port to Python 3 and Collabwrapper #20
base: master
Are you sure you want to change the base?
Conversation
Sorry about the commit messages😅 |
We can squash later. The commits show your work, which is fine to do. However, if you'd like to avoid having to use GitHub to reach your VM, you can use git over SSH between your main system and your VM, or write a script to copy your activity repository over SSH. I've not yet reviewed your changes. |
Thanks @quozl, I'll try that next time. But I will say that this was very convenient for me. |
Yes, use whatever is most convenient. I always work on my editor and local tooling so that the time between finishing a change and seeing the result is under three or four seconds. Best is half a second. I achieve this with tricks like persistent SSH sessions (ControlMaster), editor plugins, scripts, keyed access, and inotifywait combined with rsync. |
Half a second!? I think my best is ~10 seconds, typing Git credentials and all that... |
At least use an SSH agent to hold your credentials so you don't have
to type them again.
|
@quozl Would you prefer to have collab fixes in this PR or separately? |
Up to you; try to figure out what you want your reviewers to review. I'm a bit confused though. You had mentioned wanting to work on #18 but you've ended up working on #19. I don't know how that happened; there's no need to port to Python 3 in order to fix collaboration. Oh well, a sorry to @Saumya-Mishra9129, but input is welcome. Most of your changes so far are to the not-working collaboration code, so if you want a Port to Python 3 to be merged first I'd expect you to revert those. I don't know why you would want that though. |
Thanks, I'll raise a separate PR once I'm done testing. Sorry about messing up the order, I took #18's description at face value (which nudged me to #19). Could have and should have continued without porting. To clarify, my goal here was to:
I thought I'd build on top of this port... |
No worries, do what you can. Yes, the issues are inter-related, but they arose during different reviews; if you look at when the issues were created you can see them distributed through time. If only I had noticed and tied them together. We don't often get to do coherent planning. |
Thanks, @quozl, will be careful next time. Correct me if I'm wrong- you're suggesting that I get back to this PR after #19, right?
I don't understand why I'd have to revert the changes (well, at least not a major chunk); my mental model was: I have a working pipeline from changes to collaboration-related methods in this PR, and will (hopefully) figure out what messages to send and how to process them in the next. Am I headed in the wrong direction? |
@sanjay237 You said that you are removing direct use of telepathy , dbus modules and porting to Collabwrapper. Is it necessary to remove telepathy and dbus because there is TelepathGLib for python3? Why this removal required? |
I don't think so. Your branch name is port2python3, and the pull request is titled "Port to Python 3", so until you make commits that did a port to Python 3 my understanding was that this pull request was work in progress, but you had just forgotten to mark it as a draft. Up until you asked "Would you prefer to have collab fixes in this PR or separately?" I had not reviewed your changes, and when I did I found you had made collaboration changes, but hadn't mentioned any needed collaboration fixes, so I assumed you were talking about #18 because it was the only issue I knew about that was connected to collaboration. You can propose changes in any order you like, but if it were me I'd fix collaboration (#18) in Python 2 first so that when the code is ported to Python 3 you can be sure that any regression in collaboration at that point is due to the port.
Okay, I guess that means you aren't trying to fix collaboration (#18) but you really are trying to work on port to Python 3 (#19), which includes a port to TelepathyGLib. You might combine the commits from #21 and resolve the review comments I made there? Up to you. I don't think it is my place to tell people what to do; the goal is working code for users of Sugar, and porting to Python 3 or fixing collaboration are comparatively minor steps toward that goal. |
You're right, @Saumya-Mishra9129. I replaced imports to TelepathyGLib and others with collabwrapper. Easier to maintain/extend that way, what do you think? @quozl Hmm, this is intended to be a working port to Python 3 which is also a port to Collabwrapper. Correcting the title right away. I'm shifting my focus to #18, but I'd appreciate a review of this PR towards the stated goals. Stated goals:
|
Thanks. Tested d550018. Collaboration doesn't quite work right; while the same game board is shared, actions on the game board aren't shown the same way on both instances. In particular the proximity numbers are shown on only one instance. |
@ultrasanjay I agree, It would surely be easier to maintain with Collabwrapper . I reviewed your code, but collaboration is not still working fine as expected. It seems hard to maintain but hope we can make it possible together.:v: |
I've ported and tested it on Debian Buster. I have made Collabwrapper imports, but collaboration does not yet work- I'm doing that under issue #18.