Skip to content

Conversation

@A5rocks
Copy link
Contributor

@A5rocks A5rocks commented May 29, 2025

A combination of a few changes that I think make things simpler:

  • CI stuff I caught a while ago
  • mark the CFFI thing as fixed
  • fix a type hint/use _open_process to simplify type ignore (I was testing out AI models on _subprocess.py and those are the valid issues they reported!)

@A5rocks A5rocks added the skip newsfragment Newsfragment is not required label May 29, 2025
Copy link
Member

@CoolCat467 CoolCat467 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 as far as I can tell, not sure what difference between open_process and _open_process is immediately though and had a question about empty bytes vs None functionality changes

@A5rocks
Copy link
Contributor Author

A5rocks commented May 30, 2025

open_process is a complicated platform dependent type and _open_process isn't. Switching just simplifies the type ignore and removes the unused-ignore

Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

See #3264 (comment) for more discussion on structuring the CI yaml.

I'm spent on arguing about it, so don't have an opinion for now.

# 'CPython' -> '3.9.0-alpha - 3.9.X'
# 'PyPy' -> 'pypy-3.9'
python-version: ${{ fromJSON(format('["{0}", "{1}"]', format('{0}.0-alpha - {0}.X', matrix.python), matrix.python))[startsWith(matrix.python, 'pypy')] }}
python-version: '${{ matrix.python }}'
Copy link
Member

Choose a reason for hiding this comment

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

was this the logic that allowed 3.14-dev? (and the comment being outdated/incorrect) If so, and we're getting rid of it, then we should simplify continue-on-error.

Copy link
Contributor Author

@A5rocks A5rocks May 30, 2025

Choose a reason for hiding this comment

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

The new logic allows both 3.14-dev and 3.14, which will both point to the latest beta. The only difference is that one indicates that Trio doesn't support it completely yet so is allowed to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear, the comment you posted above was not possible (I didn't realized it at the time) due to the old logic here. The options are:

  • remove the old logic, keep the continue-on-error
  • keep the old logic, remove the continue-on-error

I chose the first because I think ideally we would keep e.g. 3.15-dev until 3.15 releases a beta (so failure doesn't matter), then change it to 3.15 along with any necessary changes to ensure that Trio supports 3.15.

@jakkdl
Copy link
Member

jakkdl commented May 30, 2025

@A5rocks
Copy link
Contributor Author

A5rocks commented May 30, 2025

Did you try removing the skipif from https://github.com/python-trio/trio/pull/3264/files#diff-569465da536cb2feb60e6d4707912f153c1a981dd0a08b8ed3e3bac2b7236290 i.e. 2nd item in #3267

No because I saw the back port PR isn't merged yet, but I could try that.

@A5rocks A5rocks merged commit efd785a into python-trio:main Jun 1, 2025
43 of 44 checks passed
@A5rocks A5rocks deleted the clean-up-logic branch June 1, 2025 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip newsfragment Newsfragment is not required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants