entangled / entangled.py

Python port of Entangled
Apache License 2.0
33 stars 7 forks source link

turn config into a passed over immutable object #6

Open RonnyPfannschmidt opened 1 year ago

RonnyPfannschmidt commented 1 year ago

supports #3 removes threadlocals

RonnyPfannschmidt commented 1 year ago

i learned that all tests monkeypatch a config object in a fragile way, that one is going to be a bummer

jhidding commented 1 year ago

I would love to discuss a better way to do testing of different configs. I wanted to be able to run tests like:

with config(hooks=["build"]):
    test_my_hook()

The monkey patching could be done a bit less fragile by setting the config member of the ConfigWrapper to a copy of the old config, and restore the pointer to the original one when we're done. Could you explain how your use-case is affected by the current implementation?

RonnyPfannschmidt commented 1 year ago

Mostly that when trying to remove the thread local things rapidly started to fall apart

Unfortunately i also observed that the chosen argument parsing library doesn't seem to support or even try enable passing extra objects

It's simply most fragile to have a single global configuration object that's being monkey patched into shape and personally i'd much prefer something free of actions at a distance when Hacking on stuff

jhidding commented 1 year ago

Please keep the issue focussed to a single item. I've chosen argh over click because of its simplicity and how it interacts with plain old argparse. If you have an issue with that, please exlpain clearly and give a motivating example in a separate issue, because I don't understand what you mean by "passing extra objects".

I feel your pain with regards to the config setup. I simply don't know a better way. I don't want to pass the config through every single function call, so it will have to be accessed through some global. Then to make things work in parallel with different configs, using threading.local is an ugly hack but only needed for testing. Like I said: I can make the slight improvement to make the Config class it self immutable, but I still don't understand why you need it.

RonnyPfannschmidt commented 1 year ago

a key reason why i strongly prefer never to use globals as config is, that it prevents potential bugs like

https://github.com/entangled/entangled.py/blob/d0d653dbde1cd94d343e05e9d7e10b00bb361e15/entangled/tangle.py#L102C1-L107C45

loading a default argument to a function from a global config means if the config is not properly considered stuff ends up wrong