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 #9

Merged
merged 7 commits into from Apr 1, 2020
Merged

Port to python 3 #9

merged 7 commits into from Apr 1, 2020

Conversation

JuiP
Copy link
Member

@JuiP JuiP commented Mar 31, 2020

Tested. No errors.

  • Port to python3
  • Port from GObject to GLib
  • Update to sugargame v1.3

@quozl @chimosky Review and merge :)

@JuiP
Copy link
Member Author

JuiP commented Mar 31, 2020

I could not find the instructions/ screenshots for how to use this activity. If there are any, please mention the link so that I can add it to readme.md

@ayushnawal
Copy link

Thanks @JuiP, have a look at [Jumble] in activities.sugarlabs.org, maybe you can get some help.

I will test it soon, reviewing for now

buttons.py Outdated Show resolved Hide resolved
@chimosky
Copy link
Member

chimosky commented Mar 31, 2020

Tested c593d19, works same as master.

Your Readme.md explains perfectly how to use the activity, you can look at including documentation for this activity in the help activity.

Fix what @ayushnawal pointed out and I'll merge.

@JuiP
Copy link
Member Author

JuiP commented Mar 31, 2020

Thanks @JuiP, have a look at [Jumble] in activities.sugarlabs.org, maybe you can get some help.

Done that, there was nothing!

Your Readme.md explains perfectly how to use the activity, you can look at including documentation for this activity in the help activity.

Sure!

Fix what @ayushnawal pointed out and I'll merge.

I seemed to have skipped that, when I ran 2to3 , it didnot make the variable name changes so git diff did not show me xxx_todo_changeme. Anyways, fixed in 7a470b4 . Tested; works the same, no errors.
Ready to be merged @chimosky ! Also, closes #8

@ayushnawal
Copy link

Thanks

Reviewed

As suggested you can include it in help activity.

@chimosky I guess we can close #8

@quozl
Copy link

quozl commented Apr 1, 2020

Thanks. Reviewed. utils.py is in several other activities, and everyone chooses a different variable name. This makes future maintenance harder. For test_blit, my preference was "here" and "rgb", but others have used "coordinates" and "color". The same names can be used in text_blit1; no need for unique names.

@JuiP
Copy link
Member Author

JuiP commented Apr 1, 2020

The same names can be used in text_blit1; no need for unique names.

Yes, I was thinking of giving a little more meaningful variable names but later realized "coord" and "rgb" are self explanatory, so ended up using same names elsewhere. I will change the variable name for text_blit and text_blit1 and message.

For test_blit, my preference was "here" and "rgb", but others have used "coordinates" and "color". The same names can be used in text_blit1;

I do not understand, do you want me to change the variable names to either ("coordinates" and "color") or ("here" and "rgb") for all the functions? @quozl

@quozl
Copy link

quozl commented Apr 1, 2020

I do not understand, do you want me to change the variable names to either ("coordinates" and "color") or ("here" and "rgb") for all the functions? @quozl

I've not made a comprehensive survey of the other activities, so I've no specific recommendation. The situation is already a mess. See what you can do to make future maintenance easier.

@JuiP
Copy link
Member Author

JuiP commented Apr 1, 2020

Done in 05b5862 :)
When I was looking for similar changes, found your comment on - sugarlabs/didg-art-activity#5
Went through the links and settled for ("coordinates" and "color").

@chimosky
Copy link
Member

chimosky commented Apr 1, 2020

Reviewed 05b5862, looks good.

Tested, works as expected. Thanks.

Only issue is mouse movement in the game, it's a bit slow for some reason.
I can't take a look now but might open an issue.

@chimosky chimosky merged commit afbe993 into sugarlabs:master Apr 1, 2020
@chimosky chimosky mentioned this pull request Apr 1, 2020
@JuiP JuiP deleted the python3 branch April 2, 2020 07:37
@JuiP
Copy link
Member Author

JuiP commented Apr 2, 2020

@chimosky @quozl
How do we decide if an activity is ready for a release? Any checkpoints?

@quozl
Copy link

quozl commented Apr 2, 2020

When I release something, it is a kind of social contract; I'm offering to fix it if I missed anything. So my considerations are whether fixing it now is better than causing pain to others and having to fix it later under stress. Here's some ideas;

  • for a Python 3 release, the activity should work with Sugar 0.117 on at least Fedora or Ubuntu distributions of Linux.
  • for a Python 2 release, the activity should work with Sugar 0.112 on either Fedora 18 or Ubuntu 18.04,
  • a Testing checklist is available. I'll accept suggestions for improving that list.
  • the Projects list for general maintenance, sometimes flake8, and sometimes updating any embedded libraries.

@chimosky
Copy link
Member

chimosky commented Apr 2, 2020

Personally, when I notice that there's been quite some changes in the activity and it hasn't been released, also when major changes have been made to an activity it should be released; say a port to python 3 or a port to Gtk3.

Like @quozl pointed out, if I want to release an activity I fix existing bugs before releasing.

There's also the maintainer check list, you should check it out.

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.

None yet

4 participants