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

Add support for alternatives to simplates #30

Merged
merged 5 commits into from
Sep 10, 2016
Merged

Add support for alternatives to simplates #30

merged 5 commits into from
Sep 10, 2016

Conversation

chadwhitacre
Copy link
Contributor

The simplate file format does not play will with the tooling ecosystem. This PR looks to replace simplates with multiple files per resource, while preserving filesystem dispatch. See #27.

@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented Sep 9, 2016

Here's a gist showing what Gratipay's team page would look like with multiple files (a Jinja template and ... a pyhook?) instead of a single simplate. Editing index.py immediately surfaced a few dead imports, since pyflakes could do its work. ;-)

@chadwhitacre
Copy link
Contributor Author

In other words, the HTTP method functions in the pyhook are essentially additional algorithm (state chain) functions.

@Changaco
Copy link
Member

Changaco commented Sep 9, 2016

  • Putting the variables directly in the state would lead to conflicts.
  • The python file feels like boilerplate, the only thing it's really doing is allowing you to work around Jinja's sandboxing by passing imports.

@chadwhitacre
Copy link
Contributor Author

The python file feels like boilerplate, the only thing it's really doing is allowing you to work around Jinja's sandboxing by passing imports.

Sure, in the GET case, but that's not that different than the simplate. The POST case is where more work would be done.

@Changaco
Copy link
Member

Changaco commented Sep 9, 2016

but that's not that different than the simplate

But the simplate isn't a separate file, and its implicit variable passing avoids the boilerplate.

@chadwhitacre
Copy link
Contributor Author

Just one sec, I think I have an implementation that allows simplates to remain as one option for dynamic resources. Almost have a commit here ...

@Changaco
Copy link
Member

Changaco commented Sep 9, 2016

In the example above it seems to me that you'd be better off using Mako instead of "pyhook" + Jinja:

+<%!
 """Show information about a single Team. It might be your team!
 """
 from collections import OrderedDict

 from aspen import Response
 from gratipay.utils import excerpt_intro, get_team, markdown

-[-----------------------------------------------------------------------------]
+%>
+<%

 team = get_team(state)
 banner = name = team.name
 suppress_sidebar = not(team.is_approved or user.ADMIN)
 is_team_owner = not user.ANON and team.owner == user.participant.username

-[-----------------------------------------------------------------------------]
-{% extends "templates/team-base.html" %}
+%>
+<%inherit file="templates/team-base.html"/>

+[… the rest of the Jinja template converted to Mako …]

@chadwhitacre
Copy link
Contributor Author

better off using Mako

Check out e1bb5a8. That allows us to register different dynamic classes based on the file extension, which means we could have plugins for Pyhooks, Mako, Simplates, or whatever.

@chadwhitacre
Copy link
Contributor Author

Actually, though, I think we should bake in the pyhook concept, because it's optional. If you don't want to use it, just don't have a foo.py file.

@chadwhitacre
Copy link
Contributor Author

The inverse could also be possible: only have a foo.py file.

@Changaco
Copy link
Member

Changaco commented Sep 9, 2016

Check out e1bb5a8.

Looks fine to me.

We'll need to further modify the dispatcher to detect conflicting dynamic resources.

You dropped the fs_media_type attribute on simplates, but apparently it wasn't used anywhere.

@chadwhitacre
Copy link
Contributor Author

Do you think we should try to get this done before 1.0? Seems like if we're planning to still support simplates as an option then it's lower priority than if we were going to drop simplates entirely.

@chadwhitacre
Copy link
Contributor Author

How about we try to merge this PR sooner rather than later, and then I/we can add the pyhooks feature in a later PR post-1.0?

@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented Sep 9, 2016

Gah ... or maybe pre-1.0? I think I'm going to turn next to the Flask and Django plugins, and I might rather say "pyhooks" than "simplates" in the docs for those. Hmm ...

@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented Sep 9, 2016

@Changaco Do you want to keep simplates? Could you use pyhooks to implement PAGES as you like?

@Changaco
Copy link
Member

Changaco commented Sep 9, 2016

Do you want to keep simplates?

Yes, until I've determined from experience that they can be entirely replaced by the alternatives without leading to boilerplate or other unpleasantness.

Dropping simplates now would require more work to port liberapay to something else, which would make it harder to get Pando running in production before the 1.0 announcement.

Could you use pyhooks to implement PAGES as you like?

Support for PAGES will need to be put in the Dynamic subclass that handles .py files.

Do you think we should try to get this done before 1.0?

Yes, but not as a priority, so only if we have the time.

@chadwhitacre
Copy link
Contributor Author

@kaguillera and I just worked through another example, Gratipay's reconciliation dashboard. Here's what we came up with. This illustrates nesting of pyhooks. The request processing state chain is extended with the GET from reconciliation.py when hitting all three of reconciliation.{html,csv,json}, and additionally with the GET from reconciliation.{csv,json}.py in the relevant case. Each of those second GETs updates the request processing state with a response. For the reconciliation.html case, the GET would be followed by a state chain function associated with the jinja2 extension, and that would update the state with a response.

@chadwhitacre
Copy link
Contributor Author

(The {CSV,JSON}Response classes would be adaptations of the current {csv,json}_dump renderers.)

@Changaco
Copy link
Member

Changaco commented Sep 9, 2016

As I said earlier, putting stuff in the state will lead to conflicts (remember AspenWeb/pando.py#426?).

@chadwhitacre
Copy link
Contributor Author

@Changaco Ready for review.

@Changaco Changaco changed the title Use multiple files instead of simplates Prepare to support the use multiple files instead of simplates Sep 10, 2016
@Changaco Changaco changed the title Prepare to support the use multiple files instead of simplates Add support for alternatives to simplates Sep 10, 2016
@Changaco
Copy link
Member

Looks okay to me. I've renamed the PR to match what it actually does. The changes aren't enough to implement your multiple-files idea (that would require bigger changes to the dispatcher), but they should allow implementing my original proposal from #27, as well as Mako templates, so I guess it's a good first step.

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

Successfully merging this pull request may close these issues.

2 participants