Fix problems in new NeXus workflows #77
Conversation
| class Filename(sciline.Scope[RunType, Path], Path): ... | ||
|
|
||
|
|
||
| @dataclass | ||
| class Filename(Generic[RunType]): | ||
| class NeXusFileSpec(Generic[RunType]): | ||
| value: FilePath | NeXusFile | NeXusGroup |
There was a problem hiding this comment.
@jl-wynen Wondering what you think about this. My latest attempt to have both, Union-type and (generic) type alias, by splitting it into two parts. For the common user there is a helper that forwards Filename into NeXusFileSpec.
There was a problem hiding this comment.
Of course it does not solve the str | Path issue, unless we split the types and helpers further.
There was a problem hiding this comment.
I'm not super happy because this adds another node to the graph.
There was a problem hiding this comment.
As you can guess this is due to lack of better options. Do you have any?
There was a problem hiding this comment.
We could duplicate the (current 4) providers that turn NeXusFileSpec into NeXusLocationSpec. Then the extra node is gone. Is that better?
There was a problem hiding this comment.
As you can guess this is due to lack of better options. Do you have any?
Drop Filename and auto-convert parameters to their key type on insertion. This would mean that
pl[NeXusFileSpec] = 'some/path'produces the expected NeXusFileSpec object.
I suggested this in the past but you decided against it in favour of duck-typing. I think that this precludes us from supporting static type checkers without workarounds like you implemented here.
On a side note, what we really want here is a sum-type that works both at runtime and makes mypy happy. I have been trying to create one several times but have been unsuccessful.
There was a problem hiding this comment.
I agree, we should consider your suggestion again. But as that is not a quick or easy solution, are you ok with keeping it as it is for now?
| Component = TypeVar('Component', bound=snx.NXobject) | ||
|
|
||
|
|
||
| class Filename(sciline.Scope[RunType, Path], Path): ... |
There was a problem hiding this comment.
With this, there are now to Filename types. The other one is in types.py. This seems confusing.
There was a problem hiding this comment.
Please see #75 (comment), this is the case for about 20 or so types.
This fixes a couple of problems with the new addition from #75, found when rolling this out to ESSsans (see scipp/esssans#160).