Flat and understandable API design

[JimMadge]

From discussion #1840 (comment)

Blog post with some good, clear rules.

We should aim for a structure which is flat, and "readable".
For example, the utility package currently exposes a mix of objects of different kinds and purposes. It isn't necessarily clear what you are importing,

from data_safe_haven.utility import Guid vs from data_safe_haven.annotated_types import Guid.

This isn't as important for us as it would be for a library. However, getting this right should mean we break the API less often which is good for developers.

[jemrobinson]

Copying some thoughts from the discussion on #1840.

I think we have two sort-of APIs.

1. The structure of commands under the dsh CLI app (which I think you very sensibly wanted to organise as something like dsh . This one could be structured like the DSH data model ("these things belong to the SRE...")

2. The internal API of connections between modules inside the project. This one could be structured like the Python data model ("these things are all subclasses of X").

NB. The importance of (2) is to break unnecessary connections between modules that should be encapsulated, which reduces the chance of circular dependencies.

[JimMadge]

I think I broadly agree and I'm coming around to the benefits of following this fairly strictly.

The CLI structure can (and should, because it that's how Typer works) reflect the commands package.

For no. 2 I think there may be a point where it makes sense to collect things based on a higher level structure. Just want to be careful we don't take it to a nonsensical extreme (all classes are instances of object so put them all in a package, for example).

[JimMadge]

In the case of Config, Context, I think the problem was we had something like,

# config/__init__.py

from .serialisable_config import SerialisableConfig
from .config import Config
from .context import Context

then

# config/config.py

from . import SerialisableConfig

which creates the circular dependency.

The solution there might be that the abstract classes really belong in a different module/package to the concrete classes.

[JimMadge]

Perhaps like

serialisers
├── config.py
├── context.py
└── dsh_pulumi_config.py
abstract_classes
├── serialisable_config.py
└── azure_config.py

[jemrobinson]

It's worse than that:

The inheritance structure is:

BaseModel
├ Context
├ DSHPulumiProject
└ YAMLSerialisableModel
  ├ AzureSerialisableModel
  | ├ Config
  | └ DSHPulumiConfig
  └ ContextSettings

but we can't currently separate out YAMLSerialisableModel and AzureSerialisableModel into a separate module because AzureSerialisableModel has methods that take a Context as an argument, but both Context and ContextSettings are defined in the same file.

I think I can make this work with all of the above in a config module, but I'll leave it until after #1820 is ready.

The easiest way around this is to keep all the classes listed above in one module

[JimMadge]

The easiest way around this is to keep all the classes listed above in one module

Except from or including Context and DSHPulumiProject?

[JimMadge]

Closed by #1848

[jemrobinson]

Is this actually completed @JimMadge ?

[JimMadge]

I think enough for now. Command line notwithstanding, but there is another issue for that.