-
Notifications
You must be signed in to change notification settings - Fork 41
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: user catalog #916
fix: user catalog #916
Conversation
Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
|
||
const sanitize = this.sanitize(content); | ||
this.catalog = { | ||
models: [...defaultCatalog.models, ...sanitize.models], |
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.
Here we give priority on the bundled catalog, what is user wants to override a recipe or model
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.
He can't. What would be the use case ?
ℹ️ There is no priority as it is an array. All recipe provided will appear, potential issue could append if multiple recipe/models have the same id however, do you want to fix that by giving the priority to the user's content ?
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 think we should check if there are multiple items with the same id. In the rare case that it may happen we could have issues as we manage the models as a map.
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.
He can't. What would be the use case ?
ℹ️ There is no priority as it is an array. All recipe provided will appear, potential issue could append if multiple recipe/models have the same id however, do you want to fix that by giving the priority to the user's content ?
Overriding a recipe because either you want to test a specific commit (more devs) or add a new model to a recipe
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.
If that's the goal it already works this way. If you have in the user catalog an entry with an id = one item in the default catalog you will see the custom one. Atleast for the models
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.
README.Md should be updaed as well as the file name changed
Signed-off-by: axel7083 <[email protected]>
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.
Works fine for me. There is just the edge case when there are two items having the same id to be decided how to be handled
I would be to give preference to the user one but that could be a follow up PR |
Co-authored-by: Luca Stocchi <[email protected]> Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
I updated the code to let the user's item overwrite the default items when ids are conflicting.
I also added two tests to ensure the expected behaviour. |
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.
LGTM 🚀
Signed-off-by: axel7083 <[email protected]>
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.
LGTM
What does this PR do?
See #792 (comment) for details
When we were importing a model we were copying to existing catalog, and creating a new one with the user's models in it. This lead to the issue that the catalog was not updated.
This PR fixes this issue, by having the user's catalog a complementary catalog, being merged at startup with the one provided by the extension. By this method, the user can define recipes, models or categories in a json file, and they will be added to the existing one if properly formatted.
When importing models, this file is automatically created and filled with the user's models details.
Screenshot / video of UI
Enregistrement.2024-04-17.151545.mp4
What issues does this PR fix or reference?
Fixes #792
How to test this PR?
Manually
user-catalog.json
file