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

[Python3] Add packages compatible only with python3 (part 2) #4980

Merged
merged 8 commits into from
Sep 24, 2018

Conversation

ngosang
Copy link
Contributor

@ngosang ngosang commented Aug 27, 2018

This is the first PR to make pymedusa compatible with python2 and python3 at the same time.
This PR is the 2nd in [Python3] series. Code still non working in python3.
Main discussion: #4975

You wanna help?

  • Test with python2. Everything should work as usual. Especially test strings/series with non ascii characters. Code still non working in python3.

TODO:

Libraries non compatible with python3 but the code works if you don't use that features (will be fixed in other PRs):

Disclaimer:

I'm testing careful with both python2 and python3 interpreters. With python2 you should expect the same functionally than development branch. Python3 is a long term goal, first versions won't be functional or even won't run.

@ngosang ngosang force-pushed the feature/py3_1 branch 4 times, most recently from e8ca93f to 12b12dd Compare August 27, 2018 17:39
@OmgImAlexis OmgImAlexis added Enhancement Needs review Needs testing Requires testing to make sure it's working as intended Do not merge Changelog Requires a changelog entry Needs test (Python) Needs tests added for this change (Python) Backend In progress labels Aug 27, 2018
@OmgImAlexis OmgImAlexis added this to the 0.2.10 milestone Aug 27, 2018
@OmgImAlexis OmgImAlexis requested a review from a team August 27, 2018 23:45
@ngosang ngosang changed the title [Python3] Add packages compatible only with python3 [Python3] Add packages compatible only with python3 (part 2) Aug 28, 2018
@ngosang ngosang changed the title [Python3] Add packages compatible only with python3 (part 2) [WIP][Python3] Add packages compatible only with python3 (part 2) Aug 28, 2018
@ngosang ngosang force-pushed the feature/py3_1 branch 2 times, most recently from adcc257 to f831403 Compare August 28, 2018 17:39
@ngosang ngosang force-pushed the feature/py3_1 branch 2 times, most recently from 0a48899 to 59c85e8 Compare August 29, 2018 07:53
@ngosang ngosang mentioned this pull request Aug 29, 2018
@ngosang ngosang force-pushed the feature/py3_1 branch 2 times, most recently from 9c02a1e to 4d142b1 Compare August 31, 2018 11:16
@ngosang
Copy link
Contributor Author

ngosang commented Sep 7, 2018

PR rebased. Removed changes related to unrar2 and twitter as they have been fixed in #5096 and #4990

@sharkykh I think this is ready for review. I didn't change any package for python2, just added new packages for python3 in a different folder. Python2 should work as it does now. Code still non working in python3. The first PR with python3 support is #4982

NOTE: I removed ext/_markupbase package because It's not python3 compatible and I think It's not necessary in python2 neither.

@ngosang ngosang changed the title [WIP][Python3] Add packages compatible only with python3 (part 2) [Python3] Add packages compatible only with python3 (part 2) Sep 7, 2018
@ngosang
Copy link
Contributor Author

ngosang commented Sep 14, 2018

@sharkykh in the first commit I duplicated the _configure_syspath() code in the test init file. Where we can move the shared code?

@sharkykh
Copy link
Contributor

@sharkykh I did all requested changes. I tested all in py2 & py3
The only remaining thing is the task to update the lib/ext tables in the doc. Could you help?
Current versions for py2 and py3:

  • bs4 4.6.3
  • github 1.43.1
  • yaml 3.13

Remember, after this PR you should keep 2 versions of these packages. #5200

Sorry, I still don't have time to work on the list...

@sharkykh in the first commit I duplicated the _configure_syspath() code in the test init file. Where we can move the shared code?

I don't think you need to go that far. All you have to do is:

sys.path.insert(1, os.path.abspath(os.path.join(os.path.dirname(__file__), '..')))

import medusa

Once you import medusa (or anything from medusa), all the patches are applied.
This is done in: https://github.com/pymedusa/Medusa/blob/develop/medusa/__init__.py
The medusa package usually gets imported in the actual tests anyway.

@sharkykh sharkykh removed Changelog Requires a changelog entry Needs test (Python) Needs tests added for this change (Python) labels Sep 18, 2018
@ngosang
Copy link
Contributor Author

ngosang commented Sep 19, 2018

@sharkykh Updated PR all DONE!

@p0psicles I saw your changes in #5232 but I think it's not useful. After this PR all packages are compatible with python2 and python3. Even all packages in lib folder. I prefer to show the folders in the table so, when you update github package. You remember to do it in the 2 folders.

@ngosang ngosang changed the title [WIP] [Python3] Add packages compatible only with python3 (part 2) [Python3] Add packages compatible only with python3 (part 2) Sep 19, 2018
@OmgImAlexis

This comment has been minimized.

@ngosang

This comment has been minimized.

Copy link
Contributor

@medariox medariox left a comment

Choose a reason for hiding this comment

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

The PR LGTM. Great job @ngosang 👍

@p0psicles p0psicles dismissed sharkykh’s stale review September 24, 2018 11:59

I believe all reveiw comments have been addressed

@p0psicles
Copy link
Contributor

Creating merge commit, as the commits are just to clean.

@p0psicles p0psicles merged commit 279f419 into pymedusa:develop Sep 24, 2018
@ngosang ngosang deleted the feature/py3_1 branch September 24, 2018 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concluded Enhancement Needs review Needs testing Requires testing to make sure it's working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants