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.

Sign up for GitHub

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

Jump to bottom

Support for yaml databags #912

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support for yaml databags #912

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
60a2877
Add support for yaml databags
e8johan Jun 5, 2021
41a4fec
Add tests for databags
e8johan Jun 5, 2021
bbce5e8
Fix typo
e8johan Aug 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 21 additions & 1 deletion lektor/databags.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import json
import os
from collections import OrderedDict
import yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to add PyYAML to install_requires.

(Also, should install/run pre-commit, which, via reorder-python-imports will, err, reorder these imports.)


from inifile import IniFile

Expand All @@ -12,13 +13,32 @@
from lektor.utils import resolve_dotted_value


# The ordered_load function is imported frmo stack overflow.
e8johan marked this conversation as resolved.
Show resolved Hide resolved
# By: https://stackoverflow.com/users/650222/coldfix
# License: CC-BY-SA 4.0 (https://stackoverflow.com/help/licensing)
# URL: https://stackoverflow.com/questions/5121931/in-python-how-can-you-load-yaml-mappings-as-ordereddicts/
def ordered_load(stream, Loader=yaml.SafeLoader, object_pairs_hook=OrderedDict):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest doing away with ordered_load and constructing the OrderedLoader class just once. E.g, at top level:

class _OrderedLoader(yaml.SafeLoader):
    def construct_ordered_dict(self, node):
        self.flatten_mapping(node)
        return OrderedDict(self.construct_pairs(node))

_OrderedLoader.add_constructor(
    yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG,
    _OrderedLoader.construct_ordered_dict,
)

Then, in load_databag (below), call yaml.load(stream, _OrderedLoader) directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this ordered_load hack is necessary at all (the same is written in the linked stackoverflow question). All dicts are ordered since CPython 3.6 (implementation-wise) or Python 3.7 (language spec).

Copy link
Contributor

Choose a reason for hiding this comment

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

All dicts are ordered since CPython 3.6 (implementation-wise) or Python 3.7 (language spec).

True, but there are still subtle differences, e.g. equality comparison for OrderedDict is order sensitive, while for dict it is not, reversed() iteration does not work for dict before python3.8 (but works for OrderedDict). In general, using OrderedDict seems more explicit when one really does care about order.

Currently, our load_databag is careful to parse to OrderedDicts for JSON and INI databags.

I guess I'd be happy either way, so long as we have tests that verify that order is preserved.

class OrderedLoader(Loader): # pylint: disable=too-many-ancestors
pass
def construct_mapping(loader, node):
loader.flatten_mapping(node)
return object_pairs_hook(loader.construct_pairs(node))
OrderedLoader.add_constructor(
yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG,
construct_mapping)
return yaml.load(stream, OrderedLoader)


def load_databag(filename):
try:
if filename.endswith(".json"):
with open(filename, "r") as f:
return json.load(f, object_pairs_hook=OrderedDict)
elif filename.endswith(".ini"):
return decode_flat_data(IniFile(filename).items(), dict_cls=OrderedDict)
elif filename.endswith(".yaml"):
with open(filename, "r") as f:
return ordered_load(f, yaml.SafeLoader)
Copy link
Contributor

Choose a reason for hiding this comment

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

Per suggestion in previous comment, this becomes

return yaml.load(stream, _OrderedLoader)

else:
return None
except (OSError, IOError) as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this isn't something touched by your PR, but might as well clean it up here...

This could be more cleanly written:

except FileNotFoundError:
    return None

Expand All @@ -35,7 +55,7 @@ def __init__(self, env):
self._bags = {}
try:
for filename in os.listdir(self.root_path):
if filename.endswith((".ini", ".json")):
if filename.endswith((".ini", ".json", ".yaml")):
self._known_bags.setdefault(filename.rsplit(".", -1)[0], []).append(
filename
)
Expand Down
7 changes: 7 additions & 0 deletions tests/databag-project/content/contents.lr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
_model: page
---
title: The Page Title
---
body:

This is a piece of _markdown_ body **text**.
1 change: 1 addition & 0 deletions tests/databag-project/databags/inibag.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
text = "Text from an ini databag"
1 change: 1 addition & 0 deletions tests/databag-project/databags/jsonbag.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{ "text": "Text from a json databag" }
1 change: 1 addition & 0 deletions tests/databag-project/databags/yamlbag.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
text: "Text from a yaml databag"
11 changes: 11 additions & 0 deletions tests/databag-project/models/page.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
[model]
label = {{ this.title }}

[fields.title]
label = Title
type = string
size = large

[fields.body]
label = Body
type = markdown
16 changes: 16 additions & 0 deletions tests/databag-project/templates/page.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<html>
<head>
</head>
<body>
<h1>{{ this.title }}</h1>
{{ this.body }}

<h1>Bags</h1>
<h2>inibag</h2>
<p>{{ bag('inibag.text') }}</p>
<h2>jsonbag</h2>
<p>{{ bag('jsonbag.text') }}</p>
<h2>yamlbag</h2>
<p>{{ bag('yamlbag.text') }}</p>
</body>
</html>
2 changes: 2 additions & 0 deletions tests/databag-project/test.lektorproject
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[project]
name = Databag Project
28 changes: 28 additions & 0 deletions tests/test_databag.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# coding: utf-8
import os

from lektor.builder import Builder
from lektor.db import Database
from lektor.environment import Environment
from lektor.project import Project


Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for the tests, but I would suggest these tests be converted to unit tests.

Test the data loading functionality with tests that call lektor.databags.load_databag directly.

If feeling ambitious, test the methods of the Databags class by constructing a Databags instance and calling the methods directly.

This makes it easier to check for specific issues (e.g., is the parsing to OrderedDicts working correctly?) It allows for more tests (to check various aspects of loading) while keeping total test run time low.

If you want to include an integration test to ensure that the bag() works in a template, that's okay. My personal feeling is that is the lowest priority of the tests — if that's broken, someone is likely to notice, even without the test in place.

def get_databag_builder(tmpdir):
Copy link
Contributor

Choose a reason for hiding this comment

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

As a pytest style note, this could be structured as a fixture.

@pytest.fixture
def databag_pad():
    project = Project.from_path(Path(__file__).parent / "databag-project")
    return project.make_env(load_plugins=False).new_pad()

@pytest.fixture
def databag_builder(databag_pad, tmp_path):
    return Builder(databag_pad, tmp_path / "output")

def test_databag_project_folder(databag_pad, databag_builder):
    ...

Or, perhaps, just override the project fixture (defined in conftest.py) which would allow for the direct use of the pad and builder fixtures from conftest.py:

@pytest.fixture
def project():
    return Project.from_path(Path(__file__).parent / "databag-project")

def test_databag_project_folder(pad, builder):
    ...

proj = Project.from_path(
os.path.join(os.path.dirname(__file__), "databag-project")
)
env = Environment(proj)
pad = Database(env).new_pad()

return pad, Builder(pad, str(tmpdir.mkdir("output")))


def test_databag_project_folder(tmpdir):
pad, builder = get_databag_builder(tmpdir)
prog, _ = builder.build(pad.root)
with prog.artifacts[0].open("rb") as f:
contents = f.read()
print(contents)
assert contents.find(b"Text from an ini databag") > -1
assert contents.find(b"Text from a json databag") > -1
assert contents.find(b"Text from a yaml databag") > -1