#491 Remove imports in tools/__init__.py, added new api module

[hiker]

Closes #491.

There is one cyclic dependency left:

• ToolRepository needs Compiler (to create all compiler instances), which needs
• BuildConfig (to type the config parameter), which needs
• ToolBox (to declare a parameter), which needs the
• ToolRepository (to get the default tool if a tools is missing)

~~For now, we break this cycle by importing the ToolRepository in the ToolBox when a default tool is needed, not at the top of a file.~~

~~We could introduce an abstract base class for ToolBox, or ToolRepository (and then use only the base class in declaration), that might solve it.~~ Edit: I've added an abstract base class to break the dependency.

I have also added an fab.api module, from which all functions and classes a user script needs can be imported, and have updated all scripts in run config to import everything from fap.api. I am not claiming that I have really declared all functions/classes in fab.api.

@yaswant , @t00sa , @Pierre-siddall, @jasonjunweilyu - feedback would be great. If we agree on this, I would love to get this into the next release. While this PR is kind of huge, it's all rather trivial changes only. And it would make sure all scripts developed in the near future can follow the design of importing from fab.api.

I am marking this as draft, since if this goes in, the documentation also needs to be updated, but before starting this I want to make sure we agree on this.

[hiker]

Thanks, @jasonjunweilyu . Could I please get feedback either here or in #491 about creation of the api submodule? It seems nice, but I am also happy to remove it (and leave it to the applications to know the right paths). I have converted some (all?) run_configs, so you can see the benefit for user scripts to have this feature. And, as Jason indicated, if we do this we need to add more imports there.

@yaswant , @MatthewHambley , @t00sa , @Pierre-siddall , @cameronbateman-mo - feedback appreciated - let's first agree on using api or not, then I'll either remove it or add more imports (before asking officially for review).

[hiker]

I've brought this up to main. @yaswant , @MatthewHambley , @Pierre-siddall , @t00sa - I still need some feedback if you want to go ahead with the API module (if not, I'll take that out, but leave the rest of the refactoring in place). Should go into 2.1

[hiker]

@MatthewHambley , not sure what to do about the flake8 issues. Explicitly duplicating all of v1.py in api.py seems stupid, but flake8 warns about import * and unused symbols.

I know that you consider disabling warnings bad. My suggestion would be to just use api.py in this PR, and you can introduce the API versioning as a separate issue later, and you can do it the way you prefer?

[yaswant]

@MatthewHambley , not sure what to do about the flake8 issues. Explicitly duplicating all of v1.py in api.py seems stupid, but flake8 warns about import * and unused symbols.

I know that you consider disabling warnings bad. My suggestion would be to just use api.py in this PR, and you can introduce the API versioning as a separate issue later, and you can do it the way you prefer?

@hiker apologies for jumping in, but I'd agree with @MatthewHambley's view on disabling warnings. I'm not a Python expert, but in this case could you not use something like

from fab.v1 import __all__ as _all

# Import all symbols explicitly
for _name in _all:
    globals()[_name] = getattr(__import__('fab.v1', fromlist=[_name]), _name)

[hiker]

@hiker apologies for jumping in, but I'd agree with @MatthewHambley's view on disabling warnings. I'm not a Python expert, but in this case could you not use something like

from fab.v1 import __all__ as _all

# Import all symbols explicitly
for _name in _all:
    globals()[_name] = getattr(__import__('fab.v1', fromlist=[_name]), _name)

Hi @yaswant , thanks a lot for your input, highly appreciated. I have to admit that my knowledge and understanding coding and python isn't as advanced, and I don't feel comfortable to just copy your example in. I have asked for feedback from some Python experts for an easier solution, and they suggested to use:

import fab.v1 as api

in fab/__init__.py file. This works if the FabException class is moved out of this init file (declaring classes or functions in __init__.py is something I always wanted to remove :) and I am glad to see that @t00sa did this in his Exception PR anyway :) ). Unfortunately, then mypy doesn't like from fab.api import ... (unless we add a typing information file, which kind of would defeat the purpose of an alias?).

I have instead opened #518, so someone with a deeper understanding of Python can implement this appropriately. And I have taken versioned API out of this PR (so removed v1.py), since this was really feature creep setting in, this ticket was about offering one api.py submodule. So I hope that this can be approved.

[MatthewHambley]

I know that you consider disabling warnings bad. My suggestion would be to just use api.py in this PR, and you can introduce the API versioning as a separate issue later, and you can do it the way you prefer?

In this case I think it might be appropriate to disable that specific warning in that specific place. However I am happy to make API versioning a separate issue. And I see you already have with #518.