Skip to content
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

Open
wants to merge 53 commits into
base: master
Choose a base branch
from

Conversation

sanjaymaniam
Copy link

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.

@sanjaymaniam
Copy link
Author

Sorry about the commit messages😅
I had to use this branch to interface with Sugar on VM, will squash it if that's better.

@quozl
Copy link
Contributor

quozl commented Mar 13, 2020

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.

@sanjaymaniam
Copy link
Author

Thanks @quozl, I'll try that next time. But I will say that this was very convenient for me.

@quozl
Copy link
Contributor

quozl commented Mar 13, 2020

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.

@sanjaymaniam
Copy link
Author

Half a second!? I think my best is ~10 seconds, typing Git credentials and all that...

@quozl
Copy link
Contributor

quozl commented Mar 13, 2020 via email

@sanjaymaniam
Copy link
Author

@quozl Would you prefer to have collab fixes in this PR or separately?

@quozl
Copy link
Contributor

quozl commented Mar 20, 2020

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.

@sanjaymaniam
Copy link
Author

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:

  • Port the existing Cookie Search master to Python 3
  • Remove direct use of telepathy and dbus modules, port to Collabwrapper.

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.

I thought I'd build on top of this port...

@quozl
Copy link
Contributor

quozl commented Mar 20, 2020

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.

@sanjaymaniam
Copy link
Author

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?

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.

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?

@Saumya-Mishra9129
Copy link
Member

@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?

@quozl
Copy link
Contributor

quozl commented Mar 22, 2020

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 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.

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.

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?

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.

@sanjaymaniam
Copy link
Author

sanjaymaniam commented Mar 22, 2020

@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?

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:

  • Port the existing Cookie Search master to Python 3
  • Remove direct use of telepathy and dbus modules, port to Collabwrapper.

@sanjaymaniam sanjaymaniam changed the title Port to Python 3 Port to Python 3 and Collabwrapper Mar 22, 2020
@quozl
Copy link
Contributor

quozl commented Mar 22, 2020

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.

@Saumya-Mishra9129
Copy link
Member

You're right, @Saumya-Mishra9129. I replaced imports to TelepathyGLib and others with collabwrapper. Easier to maintain/extend that way, what do you think?

@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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants