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

DOC: Order cancel_called after cancelled_caught #430

Closed
wants to merge 2 commits into from
Closed

Conversation

smurfix
Copy link
Contributor

@smurfix smurfix commented Feb 10, 2018

and add a note that you mostly don't want to use cancel_called

and add a note that you mostly don't want to use cancel_called
@njsmith
Copy link
Member

njsmith commented Feb 13, 2018

Test failures are #432 and #433.

@codecov
Copy link

codecov bot commented Feb 13, 2018

Codecov Report

Merging #430 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
- Coverage    99.2%   99.17%   -0.03%     
==========================================
  Files          89       89              
  Lines       10332    10572     +240     
  Branches      719      753      +34     
==========================================
+ Hits        10250    10485     +235     
- Misses         63       64       +1     
- Partials       19       23       +4
Impacted Files Coverage Δ
trio/tests/test_highlevel_open_unix_stream.py 98% <0%> (-2%) ⬇️
trio/tests/test_file_io.py 97.6% <0%> (-0.6%) ⬇️
trio/tests/test_path.py 100% <0%> (ø) ⬆️
trio/_core/tests/test_run.py 100% <0%> (ø) ⬆️
trio/_path.py 100% <0%> (ø) ⬆️
trio/_util.py 95.32% <0%> (+0.53%) ⬆️

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 3170205...17f9a2b. Read the comment docs.

@njsmith
Copy link
Member

njsmith commented Feb 13, 2018

Thanks! Looked good, but then I edited it a bit more :-). Feel free to merge if you like the result (and once the CI situation is sorted out...)

@njsmith
Copy link
Member

njsmith commented Feb 13, 2018

For the record, CI issues are in #434.

@njsmith
Copy link
Member

njsmith commented Feb 13, 2018

BTW, any reason in particular you're making branches in the main repo instead of your fork? I don't know that it's a problem or anything, it's just not something I've seen before...

@smurfix
Copy link
Contributor Author

smurfix commented Feb 13, 2018

Nah, no particular reason except for history; the github feature to auto-allow others to push to your PR branch didn't exist when I started working with it.

@njsmith
Copy link
Member

njsmith commented Feb 13, 2018

Cycling to force travis to redo the build. (I think I tried clicking "restart build", but it's still red for some reason... maybe "restart build" doesn't pick up the latest changes from master? Or maybe I just blanked and forgot to do it. Anyway, I know this works...)

@njsmith njsmith closed this Feb 13, 2018
@njsmith njsmith reopened this Feb 13, 2018
@njsmith
Copy link
Member

njsmith commented Feb 13, 2018

Or maybe I was getting confused between the PR build and the branch build? Well, they're re-running now anyway...

@njsmith
Copy link
Member

njsmith commented Feb 14, 2018

Ok, so the problem is that apparently that (a) because this is a branch in the main repo, Travis is running the tests twice, once using their PR testing logic, and once using their branch testing logic, (b) some interaction between github's branch protection logic and how Travis reports commit statuses means that github won't allow this to be merged unless both sets of Travis tests are passing, and (c) when Travis tests a branch, it tests it directly, instead of merging it with master first (like it does for PRs).

Conclusion: this can't be merged until someone merges from master.

@njsmith
Copy link
Member

njsmith commented Feb 14, 2018

Or, even easier, I put up a new PR (#436) to get back onto the normal workflow. Closing this in favor of that.

@njsmith njsmith closed this Feb 14, 2018
@njsmith njsmith deleted the doc branch February 14, 2018 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants