Fixes failure of the embed resolver to find common embed file in package #167
Conversation
le_utils/validators/common.py
Outdated
| with open(os.path.join(schema_dir, "definitions-embed_common.json")) as f: | ||
| common_definitions = json.load(f) | ||
| common_definitions = json.loads( | ||
| pkgutil.get_data("le_utils", "resources/embedcommondefinitions.json").decode( |
There was a problem hiding this comment.
I kinda liked the old file name, because it followed the pattern of the existing files. Did it contain disallowed characters?
There was a problem hiding this comment.
I don't think so. I guess it was more of a convention change in the context of the 'resources' folder. Will make the change and test it out.
There was a problem hiding this comment.
Made the change--works correctly.
| schema_dir = os.path.join(os.path.dirname(__file__), "..", "..", "spec") | ||
| with open(os.path.join(schema_dir, "definitions-embed_common.json")) as f: | ||
| common_definitions = json.load(f) | ||
| common_definitions = json.loads( |
There was a problem hiding this comment.
A more general question here: is there any advantage to storing the schema in JSON, rather than just inline in Python?
The way I had setup the spec folder with JSON was to then programmatically generate Python and JS code using those specs as a common base. If we don't need the schema in JS then just having it directly in Python might avoid these issues.
This also is not a blocker, as it's clearly following what we do for other constants (I just also have a general intention to migrate those to a more 'spec' style in future).
There was a problem hiding this comment.
is there any advantage to storing the schema in JSON, rather than just inline in Python?
Probably not. Might be good to do an audit to asses if the JS schemas are in use prior to the change which should be pretty straight forward, I think!
There was a problem hiding this comment.
Yeah - we don't need to make any changes here. I can write up a follow up issue for this broader question.
bjester
left a comment
bjester
left a comment
There was a problem hiding this comment.
Existing tests seem sufficient, and discussion has resolved questions about implementation
3 participants
Summary
This PR fixes an issue where the embed resolver was unable to locate a resource within the context of a python package. The fix involved moving the common embed definitions file to the resources folder and using
json.loadsin combination withpkgutil.get_data, aligning with how internal resources are accessed in other parts of the package.References
https://github.com/learningequality/studio/actions/runs/14479363608/job/40612747596?pr=5005
Reviewer guidance