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

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

Conversation

Saumya-Mishra9129
Copy link
Member

@Saumya-Mishra9129 Saumya-Mishra9129 commented May 12, 2020

@quozl @pro-panda Please review. Fixes #13

@rahul-bothra
Copy link
Contributor

Reviewed. Not tested. Looks good to me

@quozl
Copy link
Contributor

quozl commented May 14, 2020

In c4a2255 and 25d3008 please properly clean up the code that is no longer needed. These are fragments for supporting very old Python, and with our port to Python 3 guide we do not need to support Python below 3.

@Saumya-Mishra9129
Copy link
Member Author

In c4a2255 and 25d3008 please properly clean up the code that is no longer needed. These are fragments for supporting very old Python, and with our port to Python 3 guide we do not need to support Python below 3.

@quozl I think we do not even need path.py as what we use in other activities is Path class from pathlib. The path.py file is of very old version of python (maybe 2.2).

@quozl
Copy link
Contributor

quozl commented May 15, 2020

You're probably right. It was new once. Perhaps a new feature of path was needed by the original authors of this activity.

sidebar.py Outdated Show resolved Hide resolved
sidebar.py Outdated Show resolved Hide resolved
sidebar.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rahul-bothra rahul-bothra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed. Thanks

Copy link
Contributor

@rahul-bothra rahul-bothra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since bea3301 is not related to Python 2 to 3, let's cherry-pick this in a separate branch and raise a new PR for this commit

@Saumya-Mishra9129
Copy link
Member Author

Since bea3301 is not related to Python 2 to 3, let's cherry-pick this in a separate branch and raise a new PR for this commit

Yeah done with changes!

path.py is based on old Python version(2.2). os.path is what we use most in python3.
@Saumya-Mishra9129
Copy link
Member Author

In c4a2255 and 25d3008 please properly clean up the code that is no longer needed. These are fragments for supporting very old Python, and with our port to Python 3 guide we do not need to support Python below 3.

@quozl It is done in 83f8ad1 . Please Review.

@quozl
Copy link
Contributor

quozl commented May 21, 2020

Thanks. I've reviewed your changes to 83f8ad1. I'm not familiar with the activity; what testing have you done?

@Saumya-Mishra9129
Copy link
Member Author

Thanks. I've reviewed your changes to 83f8ad1. I'm not familiar with the activity; what testing have you done?

@quozl I am testing with https://wiki.sugarlabs.org/go/Activities/ShowNTell. I didn't find any other thing.

@Saumya-Mishra9129
Copy link
Member Author

@quozl Have you done testing as I mentioned here??

Thanks. I've reviewed your changes to 83f8ad1. I'm not familiar with the activity; what testing have you done?

@quozl I am testing with https://wiki.sugarlabs.org/go/Activities/ShowNTell. I didn't find any other thing.

@quozl
Copy link
Contributor

quozl commented May 28, 2020

No, I've not had time to do testing yet. Can you tell me what testing you did?

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.

Port to Python 3
3 participants