Default value for target_root, to make recipe modules importable

[cisaacstern]

Closes #587

[yuvipanda]

hmmmmm, I'm not a fan of getcwd as now behavior fundamentally differs based on where we're running this from. This leads to confusing to debug stuff...

I made a different suggestion for fixing this in #587 (comment).

[cisaacstern]

Fair enough!

What about a sentinel value (based on this SO and this PEP), something like:

class InjectableDefault:
   """Serves as a default for values which we expect to be either set or injected."""
   pass

def injectable_default():
    return InjectableDefault()

@dataclass
class StoreToZarr(...):
    ...
    target_root: Union[str, FSSpecTarget, InjectableDefault] = field(default_factory=injectable_default)

[cisaacstern]

Does that possibly make debugging clearer because it just won't work?

Trying to see if we can add this as a small patch without dropping a whole Python version support...

[cisaacstern]

A possible benefit of this InjectableDefault approach is that it provides a type hint in the code which can flag a given argument as also being declared in #566 (once that lands)? So when we read the transforms module... we know what we expect to be injected, without cross-referencing to the injection spec added by that PR...?

[yuvipanda]

Nah, we should just drop the python version support! If we want to do something with types, we can do that separately from this - but IMO in this case, the right way to tackle this is to drop 3.9 support.

[cisaacstern]

IMO in this case, the right way to tackle this is to drop 3.9 support.

Just realizing that the only thing dropping 3.9 solves is where we set the default, not what the default is.

With 3.10, we can set the default on ZarrWriterMixin and still have required fields on child classes. With 3.9, we need to set the default on the child classes.

But in either case, we need to chose what the default is. 5a37cb7 demonstrates the sentinel class idea (again, this could be thought of as agnostic to 3.9 or 3.10, we need a default either way).

I think the empty sentinel works nicely, and indeed prevents the recipe from being run in the first place.

What if we use something like this here, so we can just merge this, and drop 3.9 in a more deliberate PR?

[yuvipanda]

Let's call it RequiredAtRuntimeDefault or something of that sort?

[cisaacstern]

Let's call it RequiredAtRuntimeDefault or something of that sort?

Love it... updating now...

Edit: Will finish this later, need to go have dinner 😄

[cisaacstern]

@yuvipanda how does this look now?

[cisaacstern]

Going to merge:

✅ All checks passing
✅ All review items addressed

🙏 Thanks @yuvipanda! This is a lot better thanks to your feedback.