Skip to content

Conversation

CoolCat467
Copy link
Member

This pull request partially completes changes that were noted in #2908.

Either added on to this pull request or in another pull request, we still need to make sure to test that cython will continue working and not find these sorts of issues after they are already a problem.

Excerpt from original comment in #2908:

But it would be good to also run a test for this in CI. We do have a test for the interpreter's built-in C coroutines (test_async_function_implemented_in_C) and I [njsmith] originally assumed that would be enough, but obviously it wasn't, so I guess we need to do the legwork to install cython in CI and build a tiny Cython module.

Copy link

codecov bot commented Dec 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e317d9c) 99.67% compared to head (00c9be5) 99.67%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2911   +/-   ##
=======================================
  Coverage   99.67%   99.67%           
=======================================
  Files         115      115           
  Lines       17308    17308           
  Branches     3109     3109           
=======================================
  Hits        17252    17252           
  Misses         38       38           
  Partials       18       18           
Files Coverage Δ
src/trio/_core/_run.py 100.00% <100.00%> (ø)

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

Looks good, I think actually doing the CI work can be done later (just repurpose the already made issue).

@CoolCat467 CoolCat467 requested review from njsmith and Zac-HD January 1, 2024 19:33
@Zac-HD
Copy link
Member

Zac-HD commented Jan 3, 2024

I'd be OK with shipping this as a fix for the current problem, but I'm also keen to get some regression tests into our CI so we can tell that it's effective and notice if it breaks later. Maybe @jakkdl can help out?

@jakkdl
Copy link
Member

jakkdl commented Jan 5, 2024

I'd be OK with shipping this as a fix for the current problem, but I'm also keen to get some regression tests into our CI so we can tell that it's effective and notice if it breaks later. Maybe @jakkdl can help out?

I'll take a look at it, seems quite doable. But agree on merging this and getting CI setup in a later PR.

@CoolCat467
Copy link
Member Author

Ok, with two approvals I am going to merge this.

@CoolCat467 CoolCat467 merged commit eb32952 into python-trio:master Jan 5, 2024
@CoolCat467 CoolCat467 deleted the cython-workaround branch January 5, 2024 17:50
@jakkdl jakkdl mentioned this pull request Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants