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

Features: single lookup and debugtoolbar panel #40

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

Conversation

jvanasco
Copy link
Contributor

These are proposed in one PR, because the panel was built/used for debugging this feature (and it's generally useful).

The current implementation detail for pyramid_mako is to create a unique template lookup for each extension, and register them into Pyramid. These template lookups are only used by Pyramid's render tools though -- not by Mako itself -- so any files "included" or "inherited" by a mako template will need to be built for the current lookup -- and not leverage already being compiled for a dedicated extension lookup.

For example: if home.mako inherits from layout.mak, the current version of pyramid_mako will compile layout.mak in the template lookup dedicated to ".mako" files whether or not it exists in the separate template lookup dedicated to ".mak" file extensions.

This proposed update allows for multiple extensions to be declared at once -- sharing a single template lookup for the group of extensions.

This appears to work in light usage and tests pass, but it could definitely use some insight and think-through by others for potential edge cases or issues.

@digitalresistor
Copy link
Member

I understand why you put the two together, but I would like to see two separate PR's for them please, this makes review simpler, and allows us to discuss changes related to one single change at a time, rather than both.

@jvanasco jvanasco force-pushed the feature-single_renderer_debugtoolbar branch from d4836d7 to a1d4798 Compare November 28, 2016 20:57
@jvanasco jvanasco force-pushed the feature-single_renderer_debugtoolbar branch from a1d4798 to 2c69a91 Compare November 28, 2016 21:00
@jvanasco
Copy link
Contributor Author

The toolbar panel was included because it simply exposed the inner-workings of the mako engine, and was entirely dependent on this feature. It's been removed from the PR, but is not possible to submit in it's own PR at this time.

@digitalresistor
Copy link
Member

Once this work is merged, you'll be able to create a PR for the toolbar!

@mmerickel
Copy link
Member

Did you consider something like config.add_mako_renderer_alias(renderer='.mako', alias='.mak')? This would allow people to add their own aliases without overwriting the entire pre-existing configuration.

@jvanasco
Copy link
Contributor Author

Did you consider something like config.add_mako_renderer_alias(renderer='.mako', alias='.mak')? This would allow people to add their own aliases without overwriting the entire pre-existing configuration.

That idea didn't occur to me. I just noticed the existing implementation created two isolated environments for .mako and .mak by default, and wanted to unify them without changing the existing API.

I considered creating a single global lookup/collection and re-using that across invocations, but I thought there could exist situations where that isn't desired -- so limiting the applicable extensions for a single factory/lookup to what was explicitly declared at the time would be preferred.

@mmerickel
Copy link
Member

I would be fine with a solution that grouped the environments based on the settings prefix used. The only reason it doesn't do that already is because I didn't want to store the environments on the registry using that prefix because it was extra work at the time. Thus the first action would store the environment on the registry and the second one would find it and re-use it instead of creating a new one. I see very few problems with this as settings are intended to be global to an app, so if people are expecting the environments to be different then they should use different settings prefixes.

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.

3 participants