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

Migrating HTML reprs to jinja2 #5188

Merged
merged 15 commits into from
Aug 11, 2021
Merged

Conversation

jacobtomlinson
Copy link
Member

This PR starts the task of migrating HTML reprs over to use Jinja2 templates instead of multiline strings. There should be no changes to the reprs themselves but they should be easier to create and more maintainable going forwards.

Before

Currently we construct HTML within Python strings

class HasWhat(dict):
    """A dictionary of all workers and which keys that worker has."""

    def _repr_html_(self):
        rows = ""

        for worker, keys in sorted(self.items()):
            summary = ""
            for key in keys:
                summary += f"""<tr><td>{key}</td></tr>"""

            rows += f"""<tr>
            <td>{worker}</td>
            <td>{len(keys)}</td>
            <td>
                <details>
                <summary style='display:list-item'>Expand</summary>
                <table>
                {summary}
                </table>
                </details>
            </td>
        </tr>"""

        output = f"""
        <table>
        <tr>
            <th>Worker</th>
            <th>Key count</th>
            <th>Key list</th>
        </tr>
        {rows}
        </table>
        """

        return output

After

Instead we can use the new distributed.widgets submodule to render this from a template.

from distributed.widgets import get_template

class HasWhat(dict):
    """A dictionary of all workers and which keys that worker has."""

    def _repr_html_(self):
        return get_template("has_what.html.j2").render(has_what=self)

And the template itself gets placed in distributed/widgets/templates and is much more readable.

<table>
<tr>
    <th>Worker</th>
    <th>Key count</th>
    <th>Key list</th>
</tr>
{% for worker, keys in has_what.items() %}
    <tr>
        <td>{{ worker }}</td>
        <td>{{ keys | length }}</td>
        <td>
            <details>
            <summary style='display:list-item'>Expand</summary>
            <table>
            {% for key in keys %}
                <tr><td>{{ key }}</td></tr>
            {% endfor %}
            </table>
            </details>
        </td>
    </tr>
{% endfor %}
</table>

Copy link
Member

@fjetter fjetter left a comment

Choose a reason for hiding this comment

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

FYI jinja is already a transitive dependency via bokeh. Bokeh, however, is currently an optional dependency so this would promote jinja to a must have. I'm not too worried about this, just wanted to highlight this.

Changes otherwise look good to me

requirements.txt Show resolved Hide resolved
distributed/client.py Outdated Show resolved Hide resolved
distributed/widgets/__init__.py Outdated Show resolved Hide resolved
distributed/widgets/templates/client.html.j2 Show resolved Hide resolved
@jacobtomlinson
Copy link
Member Author

Thanks for the review @jrbourbeau. This is now complete, all HTML reprs have been migrated. Ready for final review and merge.

distributed/client.py Outdated Show resolved Hide resolved
Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @jacobtomlinson! This is in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants