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 Python3 and Update sugargame #1

Closed
wants to merge 11 commits into from
Closed

Port to Python3 and Update sugargame #1

wants to merge 11 commits into from

Conversation

Saumya-Mishra9129
Copy link

@Saumya-Mishra9129 Saumya-Mishra9129 commented Jun 4, 2020

@JuiP @srevinsaju Review. However on testing I got some errors.

Screenshot from 2020-06-04 23-43-35

Screenshot from 2020-06-04 23-42-31

JuiP and others added 11 commits June 3, 2020 15:50
- WIP on sugarlabs#19
- Fixed the calculations of hard coded coordinates. ( + 45) helps to center it
- The activity coordinates are hard coded for a system with display resolution as 1280*720. Scaling factor extends support to other screen dimensions.
- scaling factor extended support to other screen dimensions.
- known bugs: the pillar vanishes after the stick hero moves to the next pillar
@srevinsaju
Copy link

srevinsaju commented Jun 4, 2020

@Saumya-Mishra9129, the error is caused because of sugarlabs#26 which I intend to fix on sugarlabs#27

I would like you to retry without modifying the current branch, so that you can make sure that this bug does not exist with sugarlabs#27 merged

git checkout Fixes
git remote add srevinsaju https://github.com/srevinsaju/stick-hero-activity
git pull srevinsaju
git branch FixesWithFixes
git checkout FixesWithFixes
git merge srevinsaju/pygame-ss

Finally resolve the merge conflicts (if any, shouldn't be there)

What it does is to get my fork and create a grafted branch called FixesWithFixes on top of your Fixes branch so that your work remains untouched, and when sugarlabs#26 is merged before Port to Python3, (because a python2 release should happen first), it will automatically get resolved on this PR too. to get back, git checkout Fixes should solve the problem.


Good to see pycharm 😄

@Saumya-Mishra9129
Copy link
Author

@JuiP @srevinsaju Review. However on testing I got some errors.

Screenshot from 2020-06-04 23-43-35

Screenshot from 2020-06-04 23-42-31

I got another error in scorescreen.py, @srevinsaju looks like we need change there as well.

@srevinsaju
Copy link

srevinsaju commented Jun 4, 2020

@Saumya-Mishra9129 I suggest you to base your branch on top of sugarlabs/master, as @JuiP is planning (as I assume) to release the last python2 fixes for display before the release of the first python3 version of the activity. You may either wait, or base your PR to @sugarlabs, so that

  • you might get more reviewers
  • A python2 release can happen

these activities are also used by millions of OLPC laptops around the world still running python2, So it is important to create the last python2 release before python3

However, it might also create merge-conflicts.


Regarding the error, yes, it seems sensible, Thanks for testing. I will update the fix accordingly

@Saumya-Mishra9129
Copy link
Author

@Saumya-Mishra9129, the error is caused because of sugarlabs#26 which I intend to fix on sugarlabs#27

I would like you to retry without modifying the current branch, so that you can make sure that this bug does not exist with sugarlabs#27 merged

git checkout Fixes
git remote add srevinsaju https://github.com/srevinsaju/stick-hero-activity
git pull srevinsaju
git branch FixesWithFixes
git checkout FixesWithFixes
git merge srevinsaju/pygame-ss

Finally resolve the merge conflicts (if any, shouldn't be there)

What it does is to get my fork and create a grafted branch called FixesWithFixes on top of your Fixes branch so that your work remains untouched, and when sugarlabs#26 is merged before Port to Python3, (because a python2 release should happen first), it will automatically get resolved on this PR too. to get back, git checkout Fixes should solve the problem.

Good to see pycharm

I tested , getting that error in scorescreen.py now.

@Saumya-Mishra9129
Copy link
Author

Saumya-Mishra9129 commented Jun 4, 2020

@Saumya-Mishra9129 I suggest you to base your branch on top of sugarlabs/master, as @JuiP is planning (as I assume) to release the last python2 fixes for display before the release of the first python3 version of the activity. You may either wait, or base your PR to @sugarlabs, so that

  • you might get more reviewers
  • A python2 release can happen

these activities are also used by millions of OLPC laptops around the world still running python2, So it is important to create the last python2 release before python3

However, it might also create merge-conflicts.

Regarding the error, yes, it seems sensible, Thanks for testing. I will update the fix accordingly

Yeah Sure, I am planning to do so. Thanks.
However @JuiP said sugarlabs#25 (comment) that she is planning to make a new release after Port to Python3. Thats why I created this Pull request here.

@srevinsaju
Copy link

srevinsaju commented Jun 4, 2020

Yeah Sure, I am planning to do so. Thanks
Great! 😄


@Saumya-Mishra9129 Fixed it on scorescreen.py and rules.py too; Please test again

@Saumya-Mishra9129
Copy link
Author

Tested ! It worked fine.

@JuiP
Copy link
Owner

JuiP commented Jun 5, 2020

However @JuiP said sugarlabs#25 (comment) that she is planning to make a new release after Port to Python3. Thats why I created this Pull request here.

Thanks! I think we should make two releases of the activity. Making a last python2 release is important. I agree with @srevinsaju that these activities are also used by millions of OLPC laptops around the world still running python2, So it is important to create the last python2 release before python3.

For now I think we could fix bugs and errors in python2 branch and then work on the port to python3. What do you think @chimosky @quozl ?

@srevinsaju
Copy link

I guess we can close this PR. Its decided to release python2 first before python3.
And it is also less likely for those mentioned in non-sugarlabs repos to see mentions if they have configured it in their account settings. @Saumya-Mishra9129 have told that, a port to python3 will be based on sugarlabs/stick-hero-activity.

@chimosky
Copy link

chimosky commented Jun 5, 2020

@JuiP said

For now I think we could fix bugs and errors in python2 branch and then work on the port to python3. What do you think @chimosky @quozl ?

I agree, fix whatever bugs that are in the python2 version and make a release.

@srevinsaju said

I guess we can close this PR. Its decided to release python2 first before python3.

Yes it was decided to release a python2 version but I don't see a reason to close the PR.

@Saumya-Mishra9129
Copy link
Author

Yes it was decided to release a python2 version but I don't see a reason to close the PR.

Thanks @chimosky I am not closing this Pull request now. Just I will continue work here after @JuiP finishes her work of releasing a new python2 version.

@srevinsaju
Copy link

srevinsaju commented Jun 6, 2020

@chimosky , @Saumya-Mishra9129 please note that when sugarlabs#25 is merged, there is no way to merge your (@Saumya-Mishra9129's) changes to stick-hero-activity/master

To release the last python2 version, sugarlabs#25 needs to be merged to master. And that will stash your changes on the Fixes branch. Even if @JuiP merges #1 to JuiP:Fixes, Your changes will not get updated on sugarlabs#25 unless we use some dirty workaround.

Please correct me if I am wrong, and also if possible, help me understand the benefits of continuing to work in this PR which is going to be merged before a python2 release 😄

EDIT: I have tested it on https://github.com/srevinsaju/test (a dummy repo), and I could not succeed. I would love to know of the intuitive way in which this would be executed, (just to increase my git knowledge, I would like to use it elsewhere too)

EDIT 2: I created another PR, but which created an additional merge commit. That means, the net result is going to be a new PR on https://github.com/sugarlabs/stick-hero-activity which is going to be driven by JuiP and with commits by Saumya-Mishra9129

@chimosky
Copy link

chimosky commented Jun 6, 2020

@srevinsaju said

@chimosky , @Saumya-Mishra9129 please note that when sugarlabs#25 is merged, there is no way to merge your (@Saumya-Mishra9129's) changes to stick-hero-activity/master

Any changes that happen on master can be rebased on this branch, it'll look something like this;

git checkout master
git pull
git checkout Fixes
git rebase master

There shouldn't be a merge commit.

@srevinsaju said

Please correct me if I am wrong, and also if possible, help me understand the benefits of continuing to work in this PR which is going to be merged before a python2 release

They're no benefits to closing this PR as it'll have to be reopened later.

4e8f53f can be cherry-picked to sugarlabs#25 for the python2 release and with an interactive rebase can be droppped here.

@srevinsaju
Copy link

4e8f53f can be cherry-picked to sugarlabs#25 for the python2 release and with an interactive rebase can be dropped here.

Thanks 😊,

I had kept my mind very closed towards only PRs and merge, I had forgotten the permissions which you have on the original repo. Thanks for this information, forgot the merge by hand method 😇

@chimosky
Copy link

chimosky commented Jun 6, 2020

I had kept my mind very closed towards only PRs and merge, I had forgotten the permissions which you have on the original repo. Thanks for this information, forgot the merge by hand method innocent

My comment wasn't actually from that perspective, I was talking from the perspective of the person who opened the PR.

@srevinsaju
Copy link

srevinsaju commented Jun 7, 2020

Ok, I am confused now 😕


EDIT: I guess I should not be worried about where the PR is created 😉

@Saumya-Mishra9129
Copy link
Author

Actually whole reason behind opening this here was because of @JuiP comment. She said she is planning to release after python3 port. So thats why I created here in order to ease the integration process. But as now whole idea has changed, I am also confused a little about rebasing. At the end I may get conflicts while rebasing.

@srevinsaju
Copy link

But as now whole idea has changed, I am also confused a little about rebasing. At the end I may get conflicts while rebasing.

This is the part I am worried about

@JuiP
Copy link
Owner

JuiP commented Jun 7, 2020

Hi, sorry for the miscomunication. I looked at the commits here, if you think rebasing and merge conflicts will be an issue, you can run 2to3 again like in 24a0a04, shouldn't take much time and other commits which are mostly cherry-picked can be cherry-picked again. :)
Also, I don't think rebasing might be an issue.

@srevinsaju
Copy link

srevinsaju commented Jun 7, 2020

We are back at the same point we started. 😅

@Saumya-Mishra9129
Copy link
Author

Closing this, as there are lot of conflicts I faced during merge.

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