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

Option to exclude code #67

Open
johtso opened this issue Sep 26, 2020 · 10 comments
Open

Option to exclude code #67

johtso opened this issue Sep 26, 2020 · 10 comments

Comments

@johtso
Copy link

johtso commented Sep 26, 2020

Something like:

#  unasync: ignore

and / or exclusion patterns..

See https://coverage.readthedocs.io/en/coverage-5.3/excluding.html

@pquentin
Copy link
Member

Why not, but I'd be interested in seeing your use case for this

@johtso
Copy link
Author

johtso commented Sep 29, 2020

I actually realised I didn't need this functionality in the end. Possibly not so useful?

@alexdreptu
Copy link

@pquentin one example would be if __await__ is implemented in the async class, it wouldn't make sense to have it in the sync version as well.

@spyoungtech
Copy link

spyoungtech commented Jul 13, 2022

@pquentin I would find this useful for my projects... For the time being, I just post-process my code to get the same effect.

A key use-case is for when the async/sync implementation is fundamentally different... particularly when I need the resulting code to pass mypy type checking.

As a small example, I have two functions that provide sync/async implementations: async_create_process and sync_create_process.

async def async_create_process(runargs: list[str]) -> asyncio.subprocess.Process:
    return await asyncio.subprocess.create_subprocess_exec(*runargs, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE)


def sync_create_process(runargs: list[str]) -> subprocess.Popen[bytes]:
    return subprocess.Popen(runargs, stdin=subprocess.PIPE, stderr=subprocess.PIPE, stdout=subprocess.PIPE)

In my async code that gets processed by unasync, I call the async version of the function... (There's also actually a class to wrap the process objects and abstract some of the other API differences, but omitting that for brevity)

The problem is that, after rewriting I end up with this:

def sync_create_process(runargs: list[str]) -> asyncio.subprocess.Process:
    return asyncio.subprocess.create_subprocess_exec(*runargs, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE)


def sync_create_process(runargs: list[str]) -> subprocess.Popen[bytes]:
    return subprocess.Popen(runargs, stdin=subprocess.PIPE, stderr=subprocess.PIPE, stdout=subprocess.PIPE)

Technically, this code works because the second function overwrites the first one and all callsites for async_create_process now call the correct sync_create_process. However, mypy hates this, for a number of reasons. Not only does the first definition fail type checking due to the return annotation, but it also carries to mypy's inspection of all the callsites.

To fix this, I created a post-processor that looks for # unasync: remove and removes the node in the sync code.

So if I add the comment like this...

async def async_create_process(runargs: list[str]) -> asyncio.subprocess.Process: # unasync: remove

The resulting code has just the sync implementation...

def sync_create_process(runargs: list[str]) -> subprocess.Popen[bytes]:
    return subprocess.Popen(runargs, stdin=subprocess.PIPE, stderr=subprocess.PIPE, stdout=subprocess.PIPE)

And mypy is happy everywhere!

Implementing this might also be a solution/workaround for issues like #74 and #64

If anyone is interested in the (largely untested) code, it's available in this gist

@pquentin
Copy link
Member

Thanks for the detailed explanation! Yeah mypy wasn't as common when we started unasync.

I would be happy to merge a pull request to that effect. @spyoungtech Note that to reuse your code in unasync it needs to be licensed as MIT or Apache 2.

One thing I'm wondering about: both use cases in that issue are about removing a function altogether, so a single marker above that function is enough and can even cover classes I think? On the other hand, having beginning/end markers instead (unasync: off and unasync: on) would be more general but would it bring value?

@spyoungtech
Copy link

spyoungtech commented Jul 13, 2022

The way it is implemented in the gist is that it removes the entire node on the comment. Similar to how you may mark things like # noqa on any particular ast node: functions, methods, classes, or down to individual lines. Basically nukes any nodes that start on that line.

# remove entire lines:
foo: AsyncOnlyType = AsyncOnlyThing()  # unasync: remove
foo += bar + (bacon, eggs, spam)  # unasync: remove

# functions
async def foo(...): # unasync: remove
   print('this all')
   print('will be removed, in the sync version')

# remove an entire class
class AsyncFoo:  # unasync: remove
    ...

# or just specific methods:
class Bar:
    async def foo(self, ...): #unasync remove
        ...
    # or a line within a method
    def foobar(self, ...):
        asyncio.run(...) # unasync: remove

Begining/end markers might be useful for modules with a ton of module level constants if they can't just group the statements in parens.

In most cases, I would suspect something like this is possible instead of on/off:

# removes all these lines together with one comment:

from async_only import ( # unasync: remove
 a,
 b, 
 c,
)  

and that's probably a simple implementation (though I'm not sure it's the most obvious)

The code in the gist is MIT licensed, so feel free to use it. I'd also be happy to make a PR for this, but I'm not sure how to elegantly incorporate it to the current process. It also requires tokenize_rt as a dependency, fyi.

@pquentin
Copy link
Member

So yeah it looks like unasync: remove is enough!

The code in the gist is MIT licensed, so feel free to use it.

MIT is good, but Apache 2 has to be an option too, this is how all/most Trio projects are licensed. https://github.com/python-trio/unasync/blob/master/LICENSE

I'd also be happy to make a PR for this, but I'm not sure how to elegantly incorporate it to the current process.

If it works and is tested, it does not have to be super elegant? I have not looked at this at all.

It also requires tokenize_rt as a dependency, fyi.

Oh, did not know about that one, it's nice! Definitely noticed the escaped lines issue when testing unasync on the standard library. Switching to tokenize_rt would be an useful contribution in itself.

@spyoungtech
Copy link

spyoungtech commented Aug 14, 2024

For whatever it's worth, my changes in #75 have been successfully been used in my project ahk for about a year and a half now. I think unlike most users, I actually commit the generated code, so the _sync directory is present in the source tree. You can see how this feature is used, in part, to help produce code that is fully typed sync code that passes mypy checks.

Some other interesting use cases I've employed...

For example as seen here you can write distinctly different sync/async code more easily by using this to reliably know whether your method/function/whatever is running under async or not:

    def some_method(self) -> Any:
        is_async = False
        is_async = True  # unasync: remove
        if is_async:
            ...  # behavior you want when run under async variant of the class
        else:
            ...  # behavior you want when run in the synchronous variant of the class
        ...

It can be used to define things that only work after being unasynced, for example raising an error in the async API, but removing it so it runs without exception in the sync API:

    @mouse_position.setter
    def mouse_position(self, new_position: Tuple[int, int]) -> None:
        raise RuntimeError('Use of the mouse_position setter is not supported in the async API.')  # unasync: remove
        x, y = new_position
        return self.mouse_move(x=x, y=y, speed=0, relative=False)

Same idea can be used for warnings that may only apply in async code.

I think #75 is stalled, but I'm available if those changes want to be revisited.

@pquentin
Copy link
Member

Thanks for the feedback. I'm happy to include the feature, but don't have the reviewing cycles at the moment. Having someone to make CI pass (you?) and then someone to review that work would help.

@spyoungtech
Copy link

No worries @pquentin thanks for your response. I've addressed the merge conflicts and the PR is now green.

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

No branches or pull requests

4 participants