generated from ynput/ayon-addon-template
-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: product ending with _1 confuses expected files validator #125
Merged
kalisp
merged 9 commits into
develop
from
bugfix/AY-7453_Deadline-product-ending-with-_1-confuses-expected-files-validator
Feb 12, 2025
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
619ba31
Fix recalculate expected files for movies
kalisp c22ca6f
Merge branch 'develop' of https://github.com/ynput/ayon-deadline into…
kalisp f8a6e2e
Change to positive check for image type
kalisp 6c68cb1
Change to positive check for image type
kalisp b2268c6
Removed dev logging
kalisp 45da463
Merge branch 'develop' of https://github.com/ynput/ayon-deadline into…
kalisp 026da2d
Merge remote-tracking branch 'origin/bugfix/AY-7453_Deadline-product-…
kalisp 091d74d
Extension query more safe
kalisp 42695b8
Revert "Extension query more safe"
kalisp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know
representation["ext"]
is not 100% required to be set, it used to be totally valid to have representation data as e.g.{"name": "exr"}
without specifyingext
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it? I though it is requirements...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, made logic bit safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually - I did a search over the codebase and didn't quickly find any place where we're still generating it without
"ext"
. But I'm quite sure it used to be optional - but it doesnt seem to be now anymore...ext
key here: https://github.com/ynput/ayon-core/blob/9ea9e344a2e147e1681a291795f3352ea71cb5af/client/ayon_core/plugins/publish/integrate.py#L542So ignore me, definitely doesn't seem optional anymore. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that @kalisp - I'd say, feel free to revert... and then if we face errors somehow we should be fixing the
ext
requirement elsewhere most likely.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was optional, when we started to use it, but that's a long time ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there is some issue for representation
as a folder
, so what will be in those?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have answers to everything, my guess is that the
"ext"
would be filled toNone
, which should not break logic here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are still required to have
ext
plus that is currently NOT implemented so can be disregarded for this PR I'd say.