search

getavalon / core

The safe post-production pipeline - https://getavalon.github.io/2.0
MIT License
214 stars 48 forks source link

Resilience against missing paths #424

Closed mottosso closed 4 years ago

mottosso commented 4 years ago

Related to #422.

This change changes the return value of avalon.pipeline.get_representation_path(), which returns a path from (1) representation context, (2) global config or (3) representation data, in that order.

With this change, it's possible to attempt to open a missing path, which is then handled by the tool. It's also possible to copy the proposed path to the clipboard via the Loader, to see what the attempted path actually was.

Before, if a path didn't exist, there was no way of telling why, where it looked and what to do about it.

This still isn't perfect however, there's an architectural flaw; the function is almost able to operate entirely in isolation, given only the data present in the representation. That's easily testable and stable, but then there's is one feature that reaches into the database and breaks testability, which is the io.parenthood function.

What we could do instead, is separate the global config from paths stored in the representation. That way a tool can ask "Is there a path in this representation? No? Ok, look globally" and handle each error individually.

mkolar commented 4 years ago

So the changed logic now is that if path_from_representation was successful (in a sense that it returned something), we won't be checking the next option, but rely on the path returned to be correct. Right?

I don't see problem with that. The truth is that if it finds template and context, fills the path, and that pat doesn't exists it would be good to know about it rather than falling back onto baked path right away. That has already caused some minor issues for us in the past.

Thumbs up from me.

mottosso commented 4 years ago

@davidlatwe Please do! Thanks guys.

mkolar commented 4 years ago

I'm wondering why this froze in space a bit. I reckon @mottosso simply didn't find time to change it from directory to the full path? Should we just fix it and merge?

BigRoy commented 4 years ago

Should we just fix it and merge?

I suppose so, yes.

It could still be worth opening an additional issue for the future about what's stated in the initial comment:

This still isn't perfect however, there's an architectural flaw; the function is almost able to operate entirely in isolation, given only the data present in the representation. That's easily testable and stable, but then there's is one feature that reaches into the database and breaks testability, which is the io.parenthood function.

What we could do instead, is separate the global config from paths stored in the representation. That way a tool can ask "Is there a path in this representation? No? Ok, look globally" and handle each error individually.

Just to be sure we keep that in mind?

A bit unrelated, but if we're changing stuff in that area should we maybe fix the typo in the function here path_from_represenation -> path_from_representation.

mkolar commented 4 years ago

We're happy to fix this, I'm just not sure what is the best way. We can either make new PR that does the same thing without the issue of not returning the full path, or send a PR to @mottosso fork which will trickle down here.

mottosso commented 4 years ago

New PR is fine, this one is stale.