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

Fixed extension to ignore common jinja extensions #458

Merged
merged 5 commits into from
Mar 10, 2021
Merged

Fixed extension to ignore common jinja extensions #458

merged 5 commits into from
Mar 10, 2021

Conversation

vallentin
Copy link
Collaborator

Fixes #457

  • Fixed so extension() for foo.html.{jinja,jinja2,j2} resolves to html instead of {jinja,jinja2,j2}.
  • Changed to allow both path and ext, i.e. so it would allow #[template(path = "template.foo.bar.baz", ext = "html")]

This was just a quick fix, so I hope I didn't overlook anything

@vallentin vallentin requested a review from djc March 9, 2021 05:45
@djc
Copy link
Owner

djc commented Mar 9, 2021

I'd like to split the first commit into a change that separates out the abstraction logic and a separate commit that adds the Jinja extension consideration. It'd be nice to have some tests for the new version of that function, too; maybe that would be easier if it was a free function rather than a method?

@vallentin
Copy link
Collaborator Author

Just to be clear, so first commit is just fn extension() where it just returns self.path.extension(). The second commit is then changing it, to account for .jinja, etc.

Good idea with the tests. I can add a private function fn extension(&Path) in input.rs, and then test that function. Is that fine?

@djc
Copy link
Owner

djc commented Mar 9, 2021

I would incorporate the to_str() part in the extracted function immediately, which seems to match the intended contract anyway.

Private function to enable testing makes sense.

@vallentin
Copy link
Collaborator Author

vallentin commented Mar 9, 2021

Unsure if I'm misinterpreting or miscommunicated. But what I meant was, the first commit would just be:

pub fn extension(&self) -> Option<&str> {
    self.path.extension().map(|s| s.to_str().unwrap())
}

Then the second commit adds the .jinja check. That's how you wanted it split, right? :)

@djc
Copy link
Owner

djc commented Mar 9, 2021

Yup!

@vallentin
Copy link
Collaborator Author

vallentin commented Mar 10, 2021

@djc fixed it

Also added some extension tests for #[template(path = "...")] and #[template(path = "...", ext = "...")]

Edit: Weird, GitHub is showing the commits in the wrong order
Edit 2: aaand, now they're shown in the correct order

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@djc djc merged commit 7b954cd into main Mar 10, 2021
@delete-merged-branch delete-merged-branch bot deleted the ext branch March 10, 2021 12:09
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.

Proper file extension for templates for plugin integration?
2 participants