search

ynput / ayon-shotgrid

A Shotgrid Addon for the Ayon server.
Apache License 2.0
5 stars 8 forks source link

Small fixes #111

Open fabiaserra opened 3 months ago

fabiaserra commented 3 months ago

Changes

fabiaserra commented 2 months ago

This would be good to merge once task issue is resolved

what do you suggest doing?

jakubjezek001 commented 2 months ago

what do you suggest doing?

To address this, validation should occur later in the process, specifically during the validator stage, to prevent premature crashes.

fabiaserra commented 2 months ago

To address this, validation should occur later in the process, specifically during the validator stage, to prevent premature crashes.

I'm still unsure what you are asking to do. The changes on this PR don't include any validation?

BigRoy commented 2 months ago

The

To address this, validation should occur later in the process, specifically during the validator stage, to prevent premature crashes.

I'm still unsure what you are asking to do. The changes on this PR don't include any validation?

  1. The change to instance.data["task"] will error due to a key error in certain instances/cases. That should be avoided. If some other part absolute requires a task - then it should safely ignore that UNTIL the validation order so that a validator can turn it into a nice report.

  2. Another approach, less pretty to the end user, is making this a dedicated KnownPublishError directly in the Collector - it will provide the artist with a not very informative UI (mostly saying "This is not your fault") but at the very least you're able to raise an error then for the logs that make it clearer as to why that edge case may occur.

1) is preferred because you can generate a helpful response to the artist as to why something is not working as intended. But better yet, is to find a solution that doesn't require the task... because it's optional for instances, and preferably tools work with that if we can.

Does that help?

fabiaserra commented 2 months ago

Does that help?

That helps a lot yeah thank you. That requires quite a bit more work than I was intending for this PR so I'm going to remove that change and someone can address that as a separate issue. We always force having the task in our publishes so that's not really an issue in our use case

BigRoy commented 2 months ago

Thanks @fabiaserra

I've removed this line from the PR description:

@jakubjezek001 If there are issues with Shotgrid publishes when there is no explicit task set that'd now make that a separate issue to track and another PR. Making this PR ready for merging if you can test it.

mkolar commented 1 month ago

I'd be happy to merge too. Maybe @robin-ynput could have a look at testing this out?