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

WIP: Unify to a single code base compatible with both Python 2 & 3 #245

Closed
wants to merge 1 commit into from
Closed

Conversation

jdufresne
Copy link
Contributor

This removes the need to maintain two separate code bases that could
potentially drift over time in subtle ways. This greatly simplifies long
term maintenance. Now, both Python 2 and 3 share the same code with the
six package as a compatibility layer.

@ingydotnet
Copy link
Member

@jdufresne I've only skimmed the changes so far, but thanks for taking this on!

Let's see if you can get the Travis tests passing next.

BTW, How necessary is it to depend on six? I notice you mostly use it for text_type and once for 2 other things.

@dthkao
Copy link
Contributor

dthkao commented Jan 20, 2019

Just chiming in that 1) this is awesome, 2) use of the six.text_type spans quite a few files and reproducing that version-conditioned logic would mean either added verbosity or a separate 2-3 compatibility module to maintain anyway, and 3) six.with_metaclass is pretty nifty.

@nitzmahone
Copy link
Member

Yeah, this has been sorely needed for a long time- thanks!

@ingydotnet ingydotnet changed the title Unify to a single code base compatible with both Python 2 & 3 WIP: Unify to a single code base compatible with both Python 2 & 3 Jan 23, 2019
@ingydotnet
Copy link
Member

Renamed this WIP:... until it's ready to merge.

@jdufresne, @nitzmahone and I think pyyaml should vendor the six.py file as part of the pyyaml dist since it is only one file and adding dependencies to an important package that currently has none might cause trouble. You can do that or we can.

Let us know if you need help getting the tests to pass.

@rooterkyberian
Copy link

Alternative proposition: drop Python 2 altogether since its by the end of the year it will reach EoL.
Python 2 users will still be able to use outdated PyYAML releases, just like they are doing with python interpreter.

Motivation: lower maintance burden, and much lower risk of bugs.

@jdufresne
Copy link
Contributor Author

I'm am definitely open to that if the maintainers are. I agree, @nitzmahone and @ingydotnet what do you say? Can we go Python 3 only?

@ingydotnet
Copy link
Member

@jdufresne Work has begun in earnest towards a PyYAML 5.1 release which we hope will become final this month. @nitzmahone, @perlpunk and I would be excited to merge this PR if it can be completed soon enough. More public details on the release should emerge this week.

@rooterkyberian it is too early to drop Python 2 support now. The YAML projects that I am involved with strive to support language versions past EOL. So it will probably be a few years.

@ingydotnet ingydotnet added this to To Do in 5.1 Release Feb 2, 2019
lib/yaml/constructor.py Outdated Show resolved Hide resolved
@jdufresne jdufresne changed the base branch from master to release/5.1 March 9, 2019 17:50
This removes the need to maintain two separate code bases that could
potentially drift over time in subtle ways. This greatly simplifies long
term maintenance. Now, both Python 2 and 3 share the same code with the
six package as a compatibility layer.
@jdufresne
Copy link
Contributor Author

Thanks for all feedback, but I no longer have the time to finish this. If someone else is interested they can continue from my work.

@jdufresne jdufresne closed this Mar 9, 2019
@ingydotnet
Copy link
Member

@jdufresne No problem. Thank you for starting this. I expect we will get it done soon after the 5.1 release.

@ingydotnet ingydotnet added this to PRs and Notes to Consider in 5.2 Release Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
5.1 Release
Possible PRs and To Do items
5.2 Release
PRs and Notes to Consider
Development

Successfully merging this pull request may close these issues.

None yet

6 participants