Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[WIP] Fix derive of PySequenceProtocol trait #423

Merged
merged 5 commits into from
Mar 31, 2019
Merged

[WIP] Fix derive of PySequenceProtocol trait #423

merged 5 commits into from
Mar 31, 2019

Conversation

althonos
Copy link
Member

@althonos althonos commented Mar 29, 2019

As shown in #421, implementing PySequenceProtocol is not possible in v0.6.0 because the custom derive macro shuffles the argument types. This PR fixes the method types, and adds some tests to check a PySequenceProtocol implementor compiles as intended.

TODO:

  • Fix argument types of __getitem__, __setitem__, __delitem__, __repeat__, __concat__
  • Fix return types of __inplace_concat__ and __inplace_repeat__: it should be something like PyResult<&'mut Self> or PyResult<PyMutRef<Self>> since these methods do not have ownership over the Rust struct.

@althonos althonos requested a review from konstin March 29, 2019 22:35
@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #423 into master will decrease coverage by 0.25%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #423      +/-   ##
=========================================
- Coverage   87.85%   87.6%   -0.26%     
=========================================
  Files          62      62              
  Lines        3285    3331      +46     
=========================================
+ Hits         2886    2918      +32     
- Misses        399     413      +14
Impacted Files Coverage Δ
src/class/macros.rs 88.35% <100%> (ø) ⬆️
src/class/sequence.rs 80.8% <66.66%> (-15.63%) ⬇️
src/types/list.rs 97.14% <0%> (+0.12%) ⬆️
src/err.rs 62.42% <0%> (+1.81%) ⬆️

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 9f2afbc...e71af92. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #423 into master will decrease coverage by 0.25%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #423      +/-   ##
=========================================
- Coverage   87.85%   87.6%   -0.26%     
=========================================
  Files          62      62              
  Lines        3285    3331      +46     
=========================================
+ Hits         2886    2918      +32     
- Misses        399     413      +14
Impacted Files Coverage Δ
src/class/macros.rs 88.35% <100%> (ø) ⬆️
src/class/sequence.rs 80.8% <66.66%> (-15.63%) ⬇️
src/exceptions.rs 64.7% <0%> (ø) ⬆️
src/type_object.rs 88.4% <0%> (ø) ⬆️
src/types/mod.rs 100% <0%> (ø) ⬆️
src/types/list.rs 97.14% <0%> (+0.12%) ⬆️
src/err.rs 62.42% <0%> (+1.81%) ⬆️

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 9a9b519...dfd1fa5. Read the comment docs.

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 to me

@althonos
Copy link
Member Author

Rebased on latest master. CI fails because of 9a9b519.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants