Allow multi-module processing. #21
Conversation
|
Thanks for doing this (and the other one!) Just to let you know that I've seen these, and I'll try to find time to review and integrate them soon. |
| @@ -246,7 +246,8 @@ def annotation(t): | |||
| defined_type = Optional(module_reference + Suppress('.'), default=None) + typereference + Optional(size_constraint, default=None) | |||
|
|
|||
| # TODO: consider exception syntax from 24.1 | |||
| extension_marker = Unique(ELLIPSIS) | |||
| # At the moment we do not do anything with ELLIPSIS - so suppress it | |||
| extension_marker = Suppress(Unique(ELLIPSIS)) | |||
There was a problem hiding this comment.
Was this necessary for something in this patch? If not, I'd prefer it if the extension marker was still included for now, so we can dig it out of sema in codegen.
|
Nicely done! Please find lots of small comments inline, but the general approach looks good to me! I can test-run and integrate when these are fixed. |
|
Also, have you thought anything about mechanisms for testing this? |
|
Okay, your fixes/suggestions have been incorporated. |
| print(pygen.auto_generated_header()) | ||
| generate_pyasn1(module, sys.stdout) | ||
| try: | ||
| module.referenced_modules = [m for m in modules if not m is module] |
There was a problem hiding this comment.
This looks a little strange... Could you push this into build_semantic_model?
Actually, when I looked at the full flow, I have another idea:
- change resolve_type_decl to take the full module list as an argument
- pass all modules to generate_pyasn1 and pass the list on into Pyasn1Backend, so that you can provide it when you call resolve_type_decl
That way you don't have to change the module from codegen, which seems like a smell.
Also, build_semantic_model, which seems pretty abstract currently, doesn't need any special handling for referenced modules, which is primarily interesting for codegen.
And you can remove the special handling of referenced_modules in SemaNode.children! Win!
|
I like this, but I think you can simplify even more by handling referenced modules primarily in codegen. See inline comments. Also, I noticed that Finally, it would be great to have tests for this, but I'm not sure how to pull it off with the current cheap infrastructure... |
|
See what you think now - all handled in Sema.py and codegen. Thanks for the pointers! |
| @@ -474,8 +482,8 @@ def generate_OID(self): | |||
| return str(fragment) | |||
|
|
|||
|
|
|||
| def generate_pyasn1(sema_module, out_stream): | |||
| return Pyasn1Backend(sema_module, out_stream).generate_code() | |||
| def generate_pyasn1(sema_module, out_stream, referenced_modules = None): | |||
There was a problem hiding this comment.
Please get rid of the default value for referenced_modules here, we're only calling this from one place, and we have them available.
|
This looks great to me, can you please fix the nits inline, then I'm ready to merge it! |
When generating code, the list of modules is passed through Pyasn1Backend in order to allow references across modules. Modules in this list are also added to the output as import statements. The main function in pyasn1gen.py has been modified to take a command line switch to output multiple modules to separate files rather than stdout.
|
Nits fixed (I think!). I've also added cross-module referenced value resolution. |
|
Same here! I accidentally turned off GitHub notifications for a while, and didn't see this. Let me get back to you as soon as I can. |
Allow multi-module processing. Patch by James Ward.
|
Thanks, merged! |
During semantic parsing, definitions with reference to other modules
have a member variable set in the ReferencedValue class.
In order to allow base types to be determined by searching the tree
of other modules, the Module class now has a member variable
'referenced_modules' which it searches through. Modules in this list
are also added to the output as import statements.
The main function in pyasn1gen.py has been modified to take a command
line switch to output multiple modules to separate files rather than
stdout.