sugarlabs / jumble-activity

GNU General Public License v3.0
0 stars 6 forks source link

Port to python 3 #9

Closed JuiP closed 4 years ago

JuiP commented 4 years ago

Tested. No errors.

@quozl @chimosky Review and merge :)

JuiP commented 4 years ago

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 commented 4 years ago

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

chimosky commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Thanks

Reviewed

As suggested you can include it in help activity.

@chimosky I guess we can close #8

quozl commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

chimosky commented 4 years ago

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.

JuiP commented 4 years ago

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

quozl commented 4 years ago

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;

chimosky commented 4 years ago

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.