-
Notifications
You must be signed in to change notification settings - Fork 18
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
Support frameworks and manifests in ContentLoader #182
Conversation
As we move to handling multiple frameworks the `ContentLoader` needs to be able to support this. This commit changes the `ContentLoader`'s role so that it is responsible for managing multiple manifests across multiple frameworks. Rather than creating multiple `ContentLoader` instances we create one, pointing it at the root content directory (`digitalmarketplace-frameworks`). On application startup we load the manifests we need and then get builders by framework slug and manifest name. If a manifest is not found either when loading or when getting a buidler we error with a `ContentNotFound` error and should fail fast. Example: ```python loader = ContentLoader('path/to/digitalmarketplace-frameworks') loader.load_manifest('g-cloud-7', 'declaration', 'declaration') loader.get_builder('g-cloud-7', 'declaration') ``` Note that this changes `read_yaml` so that it raises an error if the file is not found. `read_yaml` only seems to be used by the content loader so this should not cause errors.
86f80bf
to
4c35de7
Compare
|
||
def get_builder(self, framework_slug, manifest): | ||
try: | ||
return ContentBuilder(self._content[framework_slug][manifest]) |
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 usually try to avoid defaultdicts since e.g. this will add a framework_slug
key to _content
whenever it's called with a missing slug. This might be not an issue for our use case, but I'd consider making load_manifest
create framework keys explicitly
Got this working locally in the supplier app. Didn't encounter any problems. I think the level of sophistication is about right when we're still dealing with a low number of frameworks. |
except KeyError: | ||
raise ContentNotFoundError("Content not found for {} and {}".format(framework_slug, manifest)) | ||
|
||
def load_manifest(self, framework_slug, manifest, question_set): |
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'd go with framework_slug, question_set, manifest
argument order, since manifests can only cover questions from one question set, and this would highlight the hierarchical order
Thinking about this some more, manifests are tied to the framework/question sets anyway, maybe the manifest file should declare this dependency instead of it being externally added by ContentLoader
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.
@allait I like your second point, eg changing the manifest from this:
-
name: Page 1
editable: True
questions:
- example
to something like this:
questions_path: declaration
sections:
-
name: Page 1
editable: True
questions:
- example
Not sure if it's something for this PR or not.
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.
First point yes, I'll do that.
Second point yes, but I didn't want to do that as part of this PR.
defaultdicts will add values if they are accessed. This means that the _content and _questions properties could grow through the course of the program running.
👍 ✨ Changelog looks great! ✨ |
@allait pointed out that manifests can only cover questions from one question set so there is a hierarchical order here.
2d24655
to
6b7f073
Compare
Support frameworks and manifests in ContentLoader
The entirety of the changes in this commit are related to the 10.0.0 change. Crown-Commercial-Service/digitalmarketplace-utils#182
As we move to handling multiple frameworks the
ContentLoader
needs to be able to support this. This commit changes theContentLoader
's role so that it is responsible for managing multiple manifests across multiple frameworks. Rather than creating multipleContentLoader
instances we create one, pointing it at the root content directory (digitalmarketplace-frameworks
). On application startup we load the manifests we need and then get builders by framework slug and manifest name. If a manifest is not found either when loading or when getting a buidler we error with aContentNotFound
error and should fail fast.Note that this changes
read_yaml
so that it raises an error if the file is not found.read_yaml
only seems to be used by the content loader so this should not cause errors.Example app change
At application startup
Old
New
In the views
Old
New