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

feature/templating-support #132

Merged
merged 12 commits into from
Dec 14, 2023
Merged

feature/templating-support #132

merged 12 commits into from
Dec 14, 2023

Conversation

ndortega
Copy link
Member

Added package extensions for Mustache.jl and OteraEngine.jl to provide better support for templating

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (77a19dd) 97.74% compared to head (833e3a6) 97.89%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
+ Coverage   97.74%   97.89%   +0.15%     
==========================================
  Files           8       12       +4     
  Lines         799      857      +58     
==========================================
+ Hits          781      839      +58     
  Misses         18       18              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ndortega
Copy link
Member Author

#133

@frankier
Copy link
Contributor

Looks nice. I took a quick look but haven't tried using it yet since Otera doesn't have includes/extends yet.

One nit I would like to pick is with the usage of runtime sniffing to determine whether a string is a file or a template literal or not and for determining fallback mimetype. Apart from incurring a small, invisible performance hit, this approach seems like it could cause confusing behaviours for end users.

For example, if I have a template and rename it, forgetting to update the code referring to it, instead of getting a "not found" error, the path will be used as a string template. In case template is not always visible, it might take longer to work out what's happening versus just getting a "path not found error". One possibility would be to just have two different functions. I suggest e.g. otera and otera_from_string, with the latter reserved for string templates.

If have a template with a HTML snippet and am somehow hitting the fallback case of automatically sniffing mime type, then I might get the correct mime type usually. However, my HTML snippet could sometimes be the empty string e.g. conditionally, and in this case it might get another mime type. This could again cause undesirable behaviour that's difficult to track down since it would not be obvious to the end user how the mime types are determined. I would suggest to not try and determine the mime type based off content at all when it comes from a template and throw an error in this case. Working based off extensions rather than content is probably a bit less surprising so the convenience factor is probably worth it there.

@frankier
Copy link
Contributor

frankier commented Nov 6, 2023

It might be an idea to use https://pkgdocs.julialang.org/dev/creating-packages/#Conditional-loading-of-code-in-packages-(Extensions) . It's faster than Requires.jl . It's possible to use both for backwards compatibility, but 1.10 will be the new LTS -- so dropping support for older Julia versions could be another possibility.

@frankier
Copy link
Contributor

frankier commented Nov 6, 2023

I can't seem to get it working with Requires.jl either at the moment. Anticipating this one though!

@frankier
Copy link
Contributor

frankier commented Nov 7, 2023

Maybe relevant if Oxygen.jl wants to be able to hook template loading MommaWatasu/OteraEngine.jl#18

@ndortega
Copy link
Member Author

ndortega commented Nov 8, 2023

Hi @frankier ,

Thanks for the feedback, you bring up some good points.

  1. I think that adding a from_file or is_file keyword argument to treat the input as a file path would be a good way forward. This way we have a uniform interface that can handle all kinds of inputs without encountering that issue you brought up. At least this way it's clear when someone is expecting to render a template from a file.
  2. I plan on keeping the mime-type sniffing just to be consistent with how responses are handled normally in the API (content is sniffed by default). Of course, if developers know the mime type then it would make sense to hard code it so there's no question even if the template is an empty string.
  3. I don't have any plans on dropping support for 1.6 anytime soon, but once 1.10 is well-established as the latest LTS I think it would make sense to drop support for older versions.

@frankier
Copy link
Contributor

Sounds good to me!

… functions

2.) unified the api between extensions, data is passed without keywords as the first argument
3.) updated templating docs
@ndortega ndortega merged commit 59a5092 into master Dec 14, 2023
10 checks passed
@ndortega ndortega deleted the feature/templating-support branch December 18, 2023 15:57
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.

2 participants