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

Many 'o' fixes #71

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Many 'o' fixes #71

wants to merge 8 commits into from

Conversation

ntninja
Copy link

@ntninja ntninja commented Dec 18, 2020

Many fixes for:

  • Running tests on Debian/Linux
  • Coverage reporting wrt Python 2 & 3 only code
  • Source code indented with tabs (like all of mine)
  • Type comments (old-style typings for compatiblity with Python 2.7 and, in some cases, 3.5)
  • Forward-references (string values in typings)
  • pathlib.Path/os.fspath filesystem paths

…hon` (b/c it is named `python3` only like in Debian)
Implementation: Detect type comments by their `# type: ` prefix, recursively unasyncify the type declaration part as new token stream, update the otherwise unchanged token value.
@codecov
Copy link

codecov bot commented Dec 18, 2020

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
Copy link
Author

ntninja commented Dec 18, 2020

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

Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

Thanks for this!

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?

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

@@ -0,0 +1,10 @@
[run]
branch=True
source=unasync
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Implementation: A context tracker monitors the incomping token stream and reports whether we are currently in a typing context or not, if we are in a typing context then the normal replacement code treats string values as a new token string to unasyncify recursively.
@ntninja
Copy link
Author

ntninja commented Dec 22, 2020

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. 😕

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
Copy link
Member

I still need to review two commits: 600807e and 4527a95. 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.)

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.

3 participants