-
-
Notifications
You must be signed in to change notification settings - Fork 108
CAPI Draft #1183
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
base: master
Are you sure you want to change the base?
CAPI Draft #1183
Conversation
The PR contains a draft for CAPI with tests. For now, only a minimal subset of functions is exposed, just for the idea demonstration. Tests are attached. @Vizonex, you might be interested in the PR. It is based on your work, but it is slightly different in implementation details. |
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (3.00%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #1183 +/- ##
==========================================
- Coverage 98.39% 97.57% -0.82%
==========================================
Files 27 28 +1
Lines 3988 4125 +137
Branches 735 736 +1
==========================================
+ Hits 3924 4025 +101
- Misses 17 52 +35
- Partials 47 48 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #1183 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The full-blown packaging setup seems like huge overengineering. There's better ways to integrate with pytest that wouldn't require additional layers of complexity.
Sorry, I don't follow your idea.
If you have a better idea, please don't hesitate to push a working code to show your thoughts. |
@asvetlov I agree with you since I spent more time than I originally anticipated trying to expose multidict's inner workings and I seem to keep having to second-guess does this go here or does this go here? At the end of the day the unpredictability of some parts of the CAPI needs a better foundation. I am willing to drop my things for a second time and just work on helping you get this passed a second time. At the end of the day. The practice I get from doing this has paid off for me in the long-run and I seem to be learning new stuff the deeper I've dived into this. I don't even feel upset at all about dropping older pull requests since I seem to keep learning new stuff. I am actually ok with closing #1178 and moving to this pull request so that we can develop a safer implementation to use. I'll hold onto a copy of my old pull request in my folders incase I need to utilize some of it's information later. |
@asvetlov I'm forking your draft instead since it has a better foundation to work upon the only thing I could see being a problem is the api calls for the capsule to be a little bit too generic since you never know if someone plans to compile this CAPI with another capsuled module. The last thing you would want is for someone to end up using the same names and colliding with it when compiling everything. |
@asvetlov Crap I just remembered I deleted my old fork I still have the code on hand and we can quickly remerge it and resume where I left off my bad. The accidental merge confused me :/ |
@asvetlov Let me write some more API Functions now that I know that you've merged my things. |
@asvetlov I made a copy of my old branch and archived it to https://github.com/Vizonex/multidict/tree/vizonex-capi so that I could resync my branch with yours again. You can use it as a reference if we need to later but let me go ahead and resync with your changes first. |
No Idea how we should go about implementing CIMultiDict and CIMultiDictProxy but now I have done everything else. |
No description provided.