Allow specifications of which modules to parse #307
Conversation
Pull Request Test Coverage Report for Build 1195
💛 - Coveralls |
There was a problem hiding this comment.
LGTM, from what I understand this argument was only used by https://github.com/adamdai/magmacam so this will be a breaking change for them, but we can post an issue to track it. Any other code, well we'll find out.
I setup the magma-lang pypi package a little while ago https://pypi.org/project/magma-lang/#history it's time to do a new release and start having dependent projects use the release version so they can track versions.
Some minor musings on portions of the code, but not blocking requests.
| modules.append(circuit) | ||
| except Exception as e: | ||
| logger.warning(f"Could not parse module {node.name} ({e}), " | ||
| f"skipping") | ||
| if module is not None: | ||
| raise Exception(f"Could not find module {module}") | ||
|
|
There was a problem hiding this comment.
This changes the behavior to return an empty list if there is no module found. Seems like reasonable behavior, but before it would raise an error if it couldn't find the module.
Either behavior seems like it could work, but maybe raising an exception if target_modules is not empty but no modules were found, might be helpful for the user (forcing them to handle this case, rather than returning an empty list which might make their code advance and cause a bug later down the line that may be harder to understand).
There was a problem hiding this comment.
Understood. Perhaps a warning would be best? The information is ultimately conveyed one way or another when an empty list is returned.
There was a problem hiding this comment.
warning sounds good, so if there is some issue caused, they can at least inspect the log and see it which might give them a clue as to where the issue is coming from
| @@ -107,7 +107,7 @@ def ParseVerilogModule(node, type_map): | |||
|
|
|||
| return node.name, args | |||
|
|
|||
| def FromVerilog(source, func, type_map, module=None): | |||
| def FromVerilog(source, func, type_map, target_modules=None): | |||
There was a problem hiding this comment.
Is it worth having both a single module argument and a list of modules argument? It would probably be convenient, but I'm not sure if there's an elegant way to do this without having to introduce some complexity in the code (handling the various cases). Maybe we can have a separate function that defines a single module from verilog (and then wraps the module in a list of one element and passes it to this function).
There was a problem hiding this comment.
Seems like reducing the complexity here and forcing the client to pass in a list and take the 0-th element of the list is preferable, esp. since it is likely not the most common case. This change also moves in the direction of thinking about bigger/general/more complex design scenarios where people might implement several modules in the same file etc.
No description provided.