GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Many 'o' fixes by ntninja · Pull Request #71 · python-trio/unasync

Conversation

@ntninja

@ntninja ntninja commented Dec 18, 2020

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ntninja added 5 commits December 18, 2020 20:43
@ntninja
Fix tests on platforms where the Python interpreter is not named `pyt…
af474c7
…hon` (b/c it is named `python3` only like in Debian)
@ntninja
Fix tests on platforms that do not pre-sort their readdir results (…
ffc5108
…e.g. Linux)
@ntninja
Skip Python 2/3 only sections on each of these to achive actual 100% …
10e449e
…coverage when running on each
@ntninja
Copy whitespace from original file to preserve spaces vs tabs
95267ba
@ntninja
Add support for type comments (PEP-484#type-comments)
600807e
Implementation: Detect type comments by their `# type: ` prefix, recursively unasyncify the type declaration part as new token stream, update the otherwise unchanged token value.
@ntninja
ntninja force-pushed the master branch from 2ef0925 to 0ab2ac2 Compare December 18, 2020 19:54
@codecov

codecov Bot commented Dec 18, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (9b15d0e) to head (3699165).
⚠️ Report is 23 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##            master       #71    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            4         4            
  Lines          254       424   +170     
  Branches        62       120    +58     
==========================================
+ Hits           254       424   +170     
Files with missing lines Coverage Δ
src/unasync/__init__.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ntninja

ntninja commented Dec 18, 2020

Copy link
Copy Markdown
Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sethmlarson
sethmlarson requested changes Dec 22, 2020
View reviewed changes

@sethmlarson sethmlarson left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread .coveragerc-py2
@@ -0,0 +1,10 @@
[run]
branch=True
source=unasync

@sethmlarson sethmlarson Dec 22, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ntninja added 3 commits December 22, 2020 18:01
@ntninja
Add support for forward-reference typings (PEP-484#forward-references)
4527a95
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
Do not fail when receiving pathlib.Path objects as filepaths anywhere
c481315
@ntninja
TEMP Try fixing Python 2 coverage as suggested
3699165
@ntninja
ntninja force-pushed the master branch from 7e303b5 to 3699165 Compare December 22, 2020 17:03
@ntninja

ntninja commented Dec 22, 2020

Copy link
Copy Markdown
Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pquentin

pquentin commented Jan 15, 2021

Copy link
Copy Markdown
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.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sethmlarson sethmlarson sethmlarson requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@ntninja @pquentin @sethmlarson
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.