Skip to content
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

Chore: Remove local settings function #83

Merged
merged 8 commits into from
Feb 20, 2024

Conversation

iLLiCiTiT
Copy link
Member

Changelog Description

Removed get_local_settings function from ayon_core.settings.

Additional info

Local settings are not implemented in AYON the same way as were in OpenPype. Some of the local settings from OpenPype are at this moment not available in AYON at all, and other ones are stored per addon. It is not possible to easily receive all the values as it was in OpenPype, at least not now, and we have to make full replacement of them in future.

Testing notes:

There is no visible change.

@iLLiCiTiT iLLiCiTiT requested a review from kalisp February 16, 2024 15:40
@ynbot ynbot added size/XS type: enhancement Improvement of existing functionality or minor addition labels Feb 16, 2024
output.update(project_locals[site_name])

return output
return {}
Copy link
Member

Choose a reason for hiding this comment

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

Should we have some warning here about the local settings not being available yet in AYON?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are TODOs all over the place, the warning would just scare artists about something they can't affect.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, maybe a debug logging for developers then?

Copy link
Member Author

@iLLiCiTiT iLLiCiTiT Feb 19, 2024

Choose a reason for hiding this comment

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

I really don't think so. For last 3 years it was used on 1 place, and I don't think someone would start to use a function with Deprecated: in docstring and empty implementation.

@tokejepsen
Copy link
Member

@jakubjezek001 any code we can run for testing?

@iLLiCiTiT
Copy link
Member Author

@jakubjezek001 any code we can run for testing?

I guess you meant me. Nothing technically changed, except of removed functions.

@kalisp
Copy link
Member

kalisp commented Feb 20, 2024

Not sure if Todos are enough, I created issue for dirmap problem, #95. More issues should be created to follow them if necessary.

Copy link
Member

@tokejepsen tokejepsen left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

@iLLiCiTiT
Copy link
Member Author

Not sure if Todos are enough, I created issue for dirmap problem, #95. More issues should be created to follow them if necessary.

I've already created tasks for that.

@iLLiCiTiT iLLiCiTiT merged commit 4edc0ab into develop Feb 20, 2024
2 checks passed
@iLLiCiTiT iLLiCiTiT deleted the enhancement/OP-8271_Remove-local-settings-function branch February 20, 2024 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS type: enhancement Improvement of existing functionality or minor addition
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants