search

python-trio / unasync

The async transformation code.
Other
91 stars 13 forks source link

Many 'o' fixes #71

Open ntninja opened 3 years ago

ntninja commented 3 years ago

Many fixes for:

codecov[bot] commented 3 years ago

Codecov Report

Merging #71 (3699165) into master (9b15d0e) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            master       #71    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            4         4            
  Lines          254       424   +170     
  Branches        62       120    +58     
==========================================
+ Hits           254       424   +170
Impacted Files Coverage Δ
src/unasync/__init__.py 100.00% <100.00%> (ø)
unasync/__init__.py 100.00% <0.00%> (ø)
ntninja commented 3 years ago

Can you make the coverage reports public please? It cannot view them and I don't see any coverage misses when running locally.

ntninja commented 3 years ago

A high level comment is it might be easier to review all of this if it were split into multiple smaller PRs. Would you be able to do that?

I'd strongly prefer not doing that, as the commits build on each other and separating them into different PRs would be a big mess. That said, each commit is self-contained and you can review them one-by-one on Github.

It looks like there was a problem shipping the coverage data? Our reports should be public as far as I'm aware? https://codecov.io/gh/python-trio/unasync/pull/71

Hmm… That worked, then stopped working, then started working again. :confused:

This might be causing a problem for Python 2 coverage, might need a [paths] source = src/unasync entry too.

I tried this in both coverage files and it did nothing. However, one can see the actual summary coverage data on CodeCov by clicking on the Files and then on the src/unasync directory which appears to be an exact mirror of the unasync directory in terms of coverage data, but is actually able to load the data.

pquentin commented 3 years ago

I still need to review two commits: https://github.com/python-trio/unasync/pull/71/commits/600807e1f43fb4401f5f3af7984e1eee0f2f908f and https://github.com/python-trio/unasync/pull/71/commits/4527a95fc65d388427cc41651c930368eb5ed4f4. The rest looks good to me!

(Sorry, it took me time to see this as I was on holidays and GitHub notifications only showed me recent items.)