Skip to content

Chore: Fix typo and typhint in get representations #195

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

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

iLLiCiTiT
Copy link
Member

Changelog Description

Define correct typehint for names_by_version_ids and fix typo representaion_names_filter > representation_names_filter.

Testing notes:

  1. Verify changes.

@iLLiCiTiT iLLiCiTiT requested review from BigRoy and kalisp October 14, 2024 11:47
@iLLiCiTiT iLLiCiTiT self-assigned this Oct 14, 2024
@iLLiCiTiT iLLiCiTiT added the type: enhancement New feature or request label Oct 14, 2024
Copy link

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside of grammar - the changes look ok. (I didn't do a test run.)

However...
However, I did want to note. Are we sure names_by_version_ids does what we want?

Passing for example:

names_by_version_ids = {
  "versionid1": ["ma", "abc"],
  "versionid2": ["exr"]
}

Are we sure the result does not include exr also for versionid1? I can't seem to find directly inside the code what makes it filter PER version id since it looks like we're just constructing a query for "representationNames" and "versionIds" and never a filter that filters representation names PER version id?

@iLLiCiTiT
Copy link
Member Author

iLLiCiTiT commented Oct 14, 2024

Are we sure the result does not include exr also for versionid1? I can't seem to find directly inside the code what makes it filter PER version id since it looks like we're just constructing a query for "representationNames" and "versionIds" and never a filter that filters representation names PER version id?

No, btw there is todo:

            Add separated function for 'names_by_version_ids' filtering.
            Because can't be combined with others.

This argument was added because of backwards compatibility with OpenPype openpype.client if I remember correctly and may be irrelevant now. Question is if we need the functionality (and if is used anywhere?).

@iLLiCiTiT iLLiCiTiT merged commit 6c949bc into develop Oct 14, 2024
2 checks passed
@iLLiCiTiT iLLiCiTiT deleted the bugfix/typos-in-get-representations branch October 14, 2024 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants