ACCESS-NRI / amami

Apache License 2.0
0 stars 0 forks source link

Remove dynamic import mechanism in parsers #41

Open truth-quark opened 1 month ago

truth-quark commented 1 month ago

The dynamic importing mechanism can introduce unintentional errors when modules are added to the commands dir. Adding modules to the command dir causes, amami to attempt to import the corresponding parser, see here:

https://github.com/ACCESS-NRI/amami/blob/main/amami/parsers/main_parser.py#L21 https://github.com/ACCESS-NRI/amami/blob/main/amami/parsers/main_parser.py#L117-L127

Problem: amami breaks if it can't find a corresponding parser for other modules. Another problem: the dynamic importing can introduce unwanted behaviour unintentionally.

Steps to fix:

atteggiani commented 1 month ago

I thought we talked about this and why I implemented it that way.

Problem: amami breaks if it can't find a corresponding parser for other modules.

This should/will not happen because we will always have a corresponding parser for each command.

Another problem: the dynamic importing can introduce unwanted behaviour unintentionally.

What unwanted behaviour?

truth-quark commented 1 month ago

I thought we talked about this and why I implemented it that way.

Yep, a few things: there's only a few commands. Does the additional complexity of a unique import implementation provide anything over direct imports?

Problem: amami breaks if it can't find a corresponding parser for other modules.

This should/will not happen because we will always have a corresponding parser for each command.

The problem is that dynamic import introduces a (currently) undocumented, artificial restriction for each module in commands to have a parser. If a module is added to the commands package (e.g. for shared coded), amami unexpectedly breaks. An empty parser module needed to be added OR changing the import mechanism.

I'd recommend a KISS approach as other developers will work with code over its lifetime. The imports can be addressed later should amami grow to having many commands.

Another problem: the dynamic importing can introduce unwanted behaviour unintentionally.

What unwanted behaviour?

It's a comment that the dynamic import creates unwanted behaviour as a side effect. I only discovered it by renaming a module during other work.

atteggiani commented 1 month ago

I don't really see "additional complexity" with a unique import (there is actually reduced complexity beause there is a lot less code to write). However, I do understand this implementation can generate issues with future development, especially if not properly documented. For now, since there are not too many commands, I think a static approach in which we write all the imports and create each of the parsers manually makes sense. As you mentioned the current dynamic approach can be reinstated later if amami grows into a bigger package with many commands (as I think and hope it will).