Switch from singleton pattern to dependency injection

[codefiles]

Singleton pattern

config.py

from pathlib import Path

class Config:
    def __init__(self, path, value):
        self.path = path
        self.value = value

config = Config(Path('/mnt'), 42)

general.py

from config import config

def use_config():
    content = (config.path / 'file').read_text()
    ...

main.py

from config import config
from general import use_config

def main():
    use_config()
    print(f'config: {config.path}, {config.value}')

if name == '__main__':
    main()

Dependency injection

config.py

class Config:
    def __init__(self, path, value):
        self.path = path
        self.value = value

general.py

def use_config(config):
    content = (config.path / 'file').read_text()
    ...

main.py

from pathlib import Path

from config import Config
from general import use_config

def main():
    config = Config(Path('/mnt'), 42)
    use_config(config)
    print(f'config: {config.path}, {config.value}')

if name == '__main__':
    main()

[svartkanin]

Generally I don't mind the pattern, with the dependency injection it becomes more tedious to pass things around though especially when the layers go deep. Also at some point it helped prevent circular dependencies.

Were you thinking of changing all singletons or just specific ones?

[codefiles]

All if possible, but handled incrementally.

[codefiles]

Now with #4148 resolved, the example script I provided there works with current master branch. That is a good start and I created this issue following that one for a reason. While main.py is a more suitable location for the script entry point than __init__.py, it was not the sole issue.

With current master branch let's examine another example script:

import argparse
from pathlib import Path

from archinstall.lib.installer import Installer
from archinstall.lib.models.device import DiskLayoutConfiguration, DiskLayoutType

def main() -> None:
	parser = argparse.ArgumentParser()
	parser.add_argument('--timezone')
	parser.add_argument('--hostname')
	parser.add_argument('mountpoint', type=Path)
	args = parser.parse_args()

	installer = Installer(
		args.mountpoint,
		DiskLayoutConfiguration(DiskLayoutType.Pre_mount),
	)

	if args.timezone:
		installer.set_timezone(args.timezone)

	if args.hostname:
		installer.set_hostname(args.hostname)

if __name__ == '__main__':
	main()

Expected usage:

# python pre_mount_cli.py --timezone UTC --hostname archlinux /mnt

Instead the output is about the same as in the example script in the other issue.

usage: pre_mount_cli.py [-h] [-v] [--config [CONFIG]] [--config-url [CONFIG_URL]] [--creds [CREDS]] [--creds-url [CREDS_URL]]
                        [--creds-decryption-key [CREDS_DECRYPTION_KEY]] [--silent] [--dry-run] [--script [SCRIPT]] [--mountpoint [MOUNTPOINT]] [--skip-ntp]
                        [--skip-wkd] [--skip-boot] [--debug] [--offline] [--no-pkg-lookups] [--plugin [PLUGIN]] [--skip-version-check] [--skip-wifi-check]
                        [--advanced] [--verbose]
pre_mount_cli.py: error: unrecognized arguments: --timezone UTC --hostname archlinux /mnt

Despite this example script not needing arch_config_handler for it is functionality and use of Installer, it will be imported by archinstall/lib/installer.py anyway. This is an example of a module-level singleton, and demonstrates that it creates import-time side effects and introduces a hidden global dependency.

A more elegant solution here would be to have an ArchConfigHandler optional dependency to Installer with fallback defaults for the four properties (silent, skip_ntp, offline, skip_wkd) it needs if a handler is not provided. That would follow the dependency injection model and is better suited for a library.

I hope this makes it clear why I proposed the switch.

[svartkanin]

Thanks for illustrating and I understand the proposal.
It's a bit difficult to say at the moment how well this will work for all cases. The device handler for example is used in a lot of different places.

Who would be the owner of these new instances? I assume someone needs to create them and then pass around?

[codefiles]

If multiple modules need access to the same instance, it should be created in their nearest common caller and passed down explicitly.

[svartkanin]

We can give it a go and see if it makes things better

[codefiles]

Great. What about #4143? It was opened before this issue but it adds a module-level singleton.

[svartkanin]

Yes I can rework it with the PR