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.

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

Get rid of signal_raise #3126

Merged
merged 3 commits into from
Nov 9, 2024
Merged

Get rid of signal_raise #3126

merged 3 commits into from
Nov 9, 2024

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Nov 1, 2024

I noticed signal.raise_signal seems to do what signal_raise used to. Let's use that instead.

@A5rocks A5rocks added the skip newsfragment Newsfragment is not required label Nov 1, 2024
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.62%. Comparing base (c7801ae) to head (980bee0).
Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3126      +/-   ##
==========================================
- Coverage   99.63%   99.62%   -0.01%     
==========================================
  Files         122      122              
  Lines       18380    18357      -23     
  Branches     1222     1221       -1     
==========================================
- Hits        18312    18289      -23     
  Misses         47       47              
  Partials       21       21              
Files with missing lines Coverage Δ
src/trio/_core/_tests/test_guest_mode.py 100.00% <100.00%> (ø)
src/trio/_core/_tests/test_ki.py 100.00% <100.00%> (ø)
src/trio/_signals.py 100.00% <100.00%> (ø)
src/trio/_tests/test_repl.py 100.00% <ø> (ø)
src/trio/_tests/test_signals.py 100.00% <100.00%> (ø)
src/trio/_tests/test_util.py 100.00% <ø> (ø)
src/trio/_util.py 100.00% <ø> (ø)

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, but wondering, why didn't we just use signal.raise_signal to begin with? Did a bug get fixed or something?

Looking into it lightly, looks like signal.raise_signal was added in 3.8, so maybe 3.7 support reasons?

@A5rocks
Copy link
Contributor Author

A5rocks commented Nov 3, 2024

I assume the code was written pre-3.8

@jakkdl
Copy link
Member

jakkdl commented Nov 3, 2024

I assume the code was written pre-3.8

yep, 2017 - 300a19d
3.7 hadn't even been released!

good find 👍

@A5rocks A5rocks enabled auto-merge (squash) November 9, 2024 06:36
@A5rocks A5rocks merged commit 5c14ddb into python-trio:main Nov 9, 2024
39 checks passed
@A5rocks A5rocks deleted the use-raise-signal branch November 9, 2024 09:49
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