Add unasync: remove feature

[spyoungtech]

Addresses: #67

Adds unasync: remove feature.

[spyoungtech]

Here's what I've got. It's worth mentioning that this feature, as currently written, can be prone to user error and result in invalid Python code.

For example, this:

a = (1 + 2

     ) + bar # unasync: remove

Will result in this:

a = (1 + 2

This could be fixed to nuke all the lines for all nodes that intersect with the comment (not just nodes that start on the same line) but I'm not sure how to do that and still preserve inline use of the feature for individual lines inside of functions/classes/methods, etc.

[spyoungtech]

Boo. It also looks like some fixes would be needed to support this feature in Python<3.8

Also worth mentioning tokenize-rt has dropped support for Python 3.6 and older (understandable, as those versions are past their EOL), so this probably wouldn't work in those versions, either.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (1b6fd0c) to head (415991b).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master       #75   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           93       108   +15     
  Branches        26        30    +4     
=========================================
+ Hits            93       108   +15     

Files	Coverage Δ
src/unasync/__init__.py	100.00% <100.00%> (ø)

[spyoungtech]

Didn't take too much time to see why travis builds are failing, but I think this could use a review before I do much more with it :)

[pquentin]

Thanks! I need to look into this more but am currently on holidays. Please ping me in two weeks if you haven't heard from me.

But yes I do intend to switch to GitHub Actions and drop support for Python 3.6.

Also, why do you only activate the remove feature starting with Python 3.8? I believe tokenize_rt supports Python 3.7. That way we could convert all our code to tokenize_rt first, then support the remove feature without parsing every file twice.

All of this is more than you signed up for: I'm not asking you to do it, I'm just mentioning the prerequisites here for the sake of completeness.

[pquentin]

@spyoungtech OK the master branch has all prerequisites now (Python 3.7+ with tokenize_rt) so we can integrate unasync: remove more deeply. If you're still interested, here's what's left to do:

• resolve the conflicts and remove the version checks
• only convert to tokens and back once by inlining unasync_remove in _unasync_tokens (or call them one after the other if a single loop isn't possible)
• only call ast.walk if a remove comment was found
• add tests in tests/data
• add a news fragment: https://github.com/python-trio/unasync/tree/master/newsfragments

[spyoungtech]

Hi @pquentin sorry for the late response. Thank you for the update and helping get those checks fixed.

Also, why do you only activate the remove feature starting with Python 3.8?

One of the issues I ran into is that, in versions of Python less than 3.8, it seems [some/all?] ast nodes do not have a end_lineno attribute that is used in the logic to find all lines to remove. You can see that in this job output: AttributeError: 'ImportFrom' object has no attribute 'end_lineno'.

That was the reason for the 3.8 version check.

I'd be happy to proceed on helping with the remaining tasks, but I'm not 100% sure I can resolve the version compatibility issue for Python3.7 with the current implementation without a great deal of effort (and significantly more complex code).

Another thought would be to replace the current approach with a simpler one, like what you suggested in #67 with a unasync equivalent of something like fmt: on/fmt: off -- key benefit being that that ast wouldn't be needed for that implementation and 3.7 compatibility would not be an issue.

I'm happy to proceed either way (applying changes you suggest, but keeping 3.8 version check for this feature) or implementing the feature as an on/off comment -- or maybe there's something else obvious I'm not considering. Let me know what you think.

[pquentin]

Oh, that makes sense. In that case I think it's fine to make that feature 3.8+ only. We should make sure to raise on Python 3.7 though (and test it). And in 11 months we can drop 3.7!

[spyoungtech]

Just wanted to chime in and say I haven't forgotten about this. It'll probably be a couple more weeks before I can get around to this (work is busy and I'm about to have a major surgery from which I will need to recover) -- but I will get around to it :-)

[spyoungtech]

Ok, so it's been almost a year on this (sorry about that) but I'm back on this now.

I've incorporated the requested changes into the code.

[spyoungtech]

welp. All checks were passing except the formatting. But applying the formatting breaks the removal tests because the resulting file after removals are processed is not black-compliant. Bit of a chicken-egg problem.

For now, I think I'll just set # fmt: off on those specific test files....

For whatever reason it also seems to have formatted another file I didn't even touch...

[spyoungtech]

Welp. Now it looks like isort has a problem and it doesn't resepect # fmt: off comments...

I'll come back to this later.

[spyoungtech]

@CoolCat467 thanks for trying to fix the merge conflicts. I ended up redoing the merge myself to fix the conflicts while (I hope) keeping the correct changes. If you're looking for your changes I force-pushed over, they can be accessed here.