-
Notifications
You must be signed in to change notification settings - Fork 59
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
chore: add cross_sync #999
base: main
Are you sure you want to change the base?
Conversation
.cross_sync/generate.py
Outdated
Render the file to a string, and optionally save to disk | ||
|
||
Args: | ||
with_black: whether to run the output through black before returning |
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.
nit: is black the formatter library? should we rename this option to with_formatter?
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.
Yeah good point, I'll change that
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.
done
.cross_sync/transformers.py
Outdated
|
||
def visit_Call(self, node): | ||
if isinstance(node.func, ast.Attribute) and isinstance(node.func.value, ast.Name) and \ | ||
node.func.attr == "rm_aio" and "CrossSync" in node.func.value.id: |
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.
I wonder if we should extract these keyword into a separate place, so it'll be easier to update the actual string if we need to?
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.
Yeah, good point, I'll see if I can clean this up a bit
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.
done
.cross_sync/transformers.py
Outdated
If found, the file is processed with the following steps: | ||
- Strip out asyncio keywords within CrossSync.rm_aio calls | ||
- transform classes and methods annotated with CrossSync decorators | ||
- classes not marked with @CrossSync.export are discarded in sync version |
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.
The comments here and the comments of this file and the read me is a bit inconsistent. Based on the code, looks like only is_async is used?
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.
Sorry, this is out of date. I ended up changing this to keep classes unchanged by default, and only dropping the ones marked with @CrossSync.drop
. I'll address this
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.
done
.cross_sync/transformers.py
Outdated
async code into sync code. | ||
|
||
At a high level: | ||
- The main entrypoint is CrossSyncClassDecoratorHandler, which is used to find classes |
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.
CrossSyncClassDecoratorHandler doesn't exist? is it AstDecorator?
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.
Sorry, this is out-dated. There used to be two separate transformers, that I combiend together into CrossSyncFileProcessor
. I'll fix the docs
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.
done
.cross_sync/generate.py
Outdated
self.tree = ast_tree | ||
self.header = header or "" | ||
|
||
def render(self, with_black=True, save_to_disk: bool = False) -> str: |
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.
should we default save_to_disk to true?
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.
I said no, because we also use this function as part of tests later, to compare to make sure the current outputs are up to date. In that case, we'd want to see the output without writing it. But I'm fine either way
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.
I think a user of the library will want to set this to true, and we can override it for testing, let's make it default to true?
eda59d4
to
54e3007
Compare
if self.sync_name: | ||
wrapped_node.name = self.sync_name | ||
# strip CrossSync decorators | ||
if hasattr(wrapped_node, "decorator_list"): |
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.
is this looking for the string "decorator_list" in the class? Is there an example you can point me to in the other PR? I also don't see this in your doc https://docs.google.com/document/d/1cy5oq4UVyg96AF8tX9nZ0zw8pbyu_lgF1DfFFegrTk0/edit?resourcekey=0-glL8ejJV1gRNXseZwYVkvw&tab=t.0#heading=h.xzptrog8pyxf
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 is part of the ast
stdlib. Some ast nodes contain a .decorator_list
attribute we can access to see the decorators associated with the class or function: https://docs.python.org/3/library/ast.html#ast.FunctionDef
if new_mapping: | ||
CrossSync.add_mapping(new_mapping, cls) | ||
if self.async_docstring_format_vars: | ||
cls.__doc__ = cls.__doc__.format(**self.async_docstring_format_vars) |
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.
what does this do? why is this only formatting async_docstring? How about sync_docstring?
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.
CrossSync allows you to add variables into docstrings, and then it will replace them with a different string for sync vs async. This came up because some async methods raise exceptions when run outside of an event loop context, but that doesn't apply to sync versions. So the docstrings are mostly the same, but with an extra little note for async
On the sync side, we can replace the variables when generating the sync file. On the async side, we update the __doc__
attribute of the class as part of the class decoration process. The code itself will have template variables when you read it directly, but docs generators will read from class.__doc__
when generating content, so it will render as expected
PR 1/x for adding the sync surface for the new data client
This class adds ast transformers for converting existing async classes into sync copies.
The conversion works through the following phases:
generate.py
is called with a path, and finds all Python files in all subdirectoriesCrossSyncClassDecoratorHandler
is called on each file, looking for any classes decorated with@CrossSync.export_sync
CrossSyncMethodDecoratorHandler
to look for any methods annotated with CrossSync decorators (CrossSunc.convert
,CrossSync.drop_method
,CrossSync.pytest
, etc).RmAioFunctions
to strip out any asyncio keywords annotated withCrossSync.rmaio()
SymbolReplacer
to replaceCrossSync
withCrossSync._Sync_Impl
in the codebase, and other user-specified symbol transformatioonsNext PR will add annotations to existing code to guide transformations