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.

Sign up for GitHub

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

Jump to bottom

Drop support for python2 #411

Merged
merged 3 commits into from
Mar 29, 2019
Merged

Drop support for python2 #411

merged 3 commits into from
Mar 29, 2019
+342 −6,170

Conversation

ijl
Copy link
Contributor

@ijl ijl commented Mar 21, 2019

This closes #138. It depends on #404.

@codecov
Copy link

codecov bot commented Mar 21, 2019

Codecov Report

Merging #411 into master will decrease coverage by 0.23%.
The diff coverage is 91.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
- Coverage   88.11%   87.88%   -0.24%     
==========================================
  Files          83       62      -21     
  Lines        3593     3285     -308     
==========================================
- Hits         3166     2887     -279     
+ Misses        427      398      -29
Impacted Files Coverage Δ
src/types/module.rs 94.28% <ø> (ø) ⬆️
src/freelist.rs 84.44% <ø> (+7.25%) ⬆️
src/class/number.rs 94.2% <ø> (-0.82%) ⬇️
src/class/sequence.rs 96.42% <ø> (-0.59%) ⬇️
src/class/pyasync.rs 100% <ø> (ø) ⬆️
src/types/mod.rs 100% <ø> (ø) ⬆️
src/err.rs 60.6% <ø> (ø) ⬆️
src/exceptions.rs 64.7% <ø> (ø) ⬆️
src/lib.rs 0% <ø> (ø) ⬆️
src/derive_utils.rs 61.42% <ø> (ø) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f75f742...6da46a4. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 21, 2019 •
edited
Loading

Codecov Report

Merging #411 into master will decrease coverage by 0.2%.
The diff coverage is 91.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
- Coverage   88.06%   87.85%   -0.21%     
==========================================
  Files          83       62      -21     
  Lines        3593     3285     -308     
==========================================
- Hits         3164     2886     -278     
+ Misses        429      399      -30
Impacted Files Coverage Δ
src/types/module.rs 94.28% <ø> (ø) ⬆️
src/freelist.rs 84.44% <ø> (+7.25%) ⬆️
src/class/basic.rs 91.07% <ø> (-0.9%) ⬇️
src/class/number.rs 94.2% <ø> (-0.82%) ⬇️
src/class/sequence.rs 96.42% <ø> (-0.59%) ⬇️
src/class/pyasync.rs 100% <ø> (ø) ⬆️
src/types/mod.rs 100% <ø> (ø) ⬆️
src/err.rs 60.6% <ø> (+1.21%) ⬆️
src/exceptions.rs 64.7% <ø> (ø) ⬆️
src/lib.rs 0% <ø> (ø) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc7776e...80179be. Read the comment docs.

konstin
konstin reviewed Mar 21, 2019
View reviewed changes
Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

Thank you!

Looks good, left some minor comments. As you've said this is blocked on v0.6/#404

Cargo.toml
@@ -39,9 +39,6 @@ version_check = "0.1.5"
[features]
default = []

# Use this feature when building python2 binding.
python2 = []

# Use this feature when building python3 binding.
python3 = []
Copy link
Member

Choose a reason for hiding this comment

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

We don't need a python3 without the python2 feature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setuptools-rust apparently assumes it. I've left it in for now with a note.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right. I've created PyO3/setuptools-rust#53 for fixing that long term, but for now a comment is good

guide/src/class.md Outdated

* `fn __unicode__(&self) -> PyResult<PyUnicode>`

On Python 2.7, provides the conversion to `unicode`.
On Python 3.x, `__unicode__` is allowed but has no effect.
`__unicode__` is allowed but has no effect.
Copy link
Member

Choose a reason for hiding this comment

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

Please add a note that this exists (only?) for historical reasons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just removed __unicode__.

@ijl
Copy link
Contributor Author

ijl commented Mar 28, 2019

@konstin please take a look.

@konstin
Copy link
Member

konstin commented Mar 28, 2019

Please run cargo fmt, that should make ci pass

@ijl
Copy link
Contributor Author

ijl commented Mar 29, 2019

@konstin ok, the build's green.

@konstin
Copy link
Member

konstin commented Mar 29, 2019

Thank you!

@konstin konstin merged commit 9f2afbc into PyO3:master Mar 29, 2019
@ijl ijl deleted the rm-python2 branch March 31, 2019 19:54
@poljar poljar mentioned this pull request Jun 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konstin konstin konstin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Python2 support
2 participants
@ijl @konstin