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

Refactoring functions and Fix Code Expression #73

Closed
wants to merge 4 commits into from
Closed

Refactoring functions and Fix Code Expression #73

wants to merge 4 commits into from

Conversation

yezz123
Copy link
Member

@yezz123 yezz123 commented Sep 16, 2021

Hello @mcmtroffaes 👋🏻
I just refactor some functions in the code itself and edit the test relate to testing the pathlib.

  • Replace range(0, x) with range(x).
  • Replace unneeded comprehension with generator.

@yezz123 yezz123 requested a review from mcmtroffaes September 16, 2021 10:07
@yezz123 yezz123 self-assigned this Sep 16, 2021
@yezz123 yezz123 marked this pull request as draft September 16, 2021 10:11
@mcmtroffaes
Copy link
Collaborator

Wow, thanks for the contribution, I very much appreciate your enthusiasm! However, there are two issues:

  1. Many parts of your patch break 2.7 compatibility (as shown by the regression testing). So at least for now I suggest holding off on this, until it's clear whether or not another 2.7 release needs to be made (I think not but I don't want to jump the gun before allowing a wider discussion). See dropping python 2.7 #71.
  2. So far, the spirit of the library has been to keep a minimal delta with upstream cpython in order to make it as easy as possible to track upstream. Whence, to modernize for Python 3, I think it would be less work to simply make a fresh copy from cpython pathlib, and then ensure compatibility with Python 3.6+. In particular, any refactoring the code should ideally happen in cpython pathlib, and not in pathlib2 here.

@yezz123
Copy link
Member Author

yezz123 commented Sep 16, 2021

@mcmtroffaes I agree with the same idea in this issue #71!

  • I just check some parts of the documentation, relate to Python2 https://www.python.org/doc/sunset-python-2, I guess even in the docs the Python2.7 is not supported in the maintenance, so multiple environments now migrate directly to Python3.x.

As of January 1st, 2020 no new bug reports, fixes, or changes will be made to Python 2, and Python 2 is no longer supported.

  • Also, about the idea of refactoring the default pathlib in CPython, this gonna be somehow hard and need a full review in multiple environments, maybe also changing the language pragmatics, and I guess this is why Pathlib 2 is here to give a great addon with more features than the default one.

@mcmtroffaes
Copy link
Collaborator

Also, about the idea of refactoring the default pathlib in CPython, this gonna be somehow hard and need a full review in multiple environments, maybe also changing the language pragmatics, and I guess this is why Pathlib 2 is here to give a great addon with more features than the default one.

Yes, going forward, maybe that could be a future role for pathlib2 if that's desired and agreed. However, I'm definitely not the right person to shepherd such developments. The point I want to get across is that historically that's not been the case and it's been an explicit "goal" of the project not to add features that weren't in cpython: pathlib has been the upstream for pathlib2, not the other way around.

@graingert
Copy link
Member

Also, about the idea of refactoring the default pathlib in CPython, this gonna be somehow hard and need a full review in multiple environments, maybe also changing the language pragmatics, and I guess this is why Pathlib 2 is here to give a great addon with more features than the default one.

Yes, going forward, maybe that could be a future role for pathlib2 if that's desired and agreed. However, I'm definitely not the right person to shepherd such developments. The point I want to get across is that historically that's not been the case and it's been an explicit "goal" of the project not to add features that weren't in cpython: pathlib has been the upstream for pathlib2, not the other way around.

my 'vision' is for this project to generally follow the example of contextlib2, eg just copy/paste updates from cpython's 'main' branch and have a minimal diff to make it work on the oldest supported (currently py3.6+) python

Historically contextlib2 was the upstream and stdlib contextlib was the downstream, but this has now mostly reversed see jazzband/contextlib2#31

@mcmtroffaes
Copy link
Collaborator

Sounds good, @graingert.

@mcmtroffaes mcmtroffaes mentioned this pull request Sep 17, 2021
Copy link
Collaborator

@mcmtroffaes mcmtroffaes left a comment

Choose a reason for hiding this comment

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

I guess we can close this (in favour of #75)?

@yezz123 yezz123 closed this Feb 5, 2022
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.

3 participants