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

Implement liquid::parse_file(...) method #58

Merged
merged 1 commit into from
Aug 16, 2016

Conversation

pop
Copy link
Contributor

@pop pop commented Aug 14, 2016

Hey I really like your project and I'm glad to be able to contribute.

If you have any questions about what / why I did something please ask. I'm fairly new to Rust so there are a few 'decisions' which were really 'I think this is how to accomplish that thing I want to do but who knows'.

Implements basic expected functionality:

  • Takes as arument a filepath and LiquidOptions.
  • Returns Result<Template>
    Includes basics tests (extends fixtures.rs).
    Includes doc-string documentation.
    fixes Add a from_file function #57

/// let output = template.render(&mut data);
/// assert_eq!(output.unwrap(), Some("Liquid! 2".to_string()));
/// ```
pub fn parse_file(fp: &str, options: LiquidOptions) -> Result<Template> {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use AsRef to enable automatic conversion of supported types to Path, which allows for a more flexible API. Basically just copy the function signature of File::open.

@johannhof
Copy link
Contributor

Hey, thanks so much for contributing! This patch looks great!

A note on the tests: I'm not sure fixtures.rs is the best place for testing the parse_file method. Why don't you make a new test file tests/parse_file.rs where you check that parse_file works correctly. Testing a couple of fixtures and checking the error cases (nonexistent file) should be enough :)

Cheers!

@pop pop changed the title Implement liquid::parse_from_file(...) method Implement liquid::parse_file(...) method Aug 15, 2016
Implements basic expected functionality:
- Takes as arument a filepath (&str) and LiquidOptions.
- Returns Result<Template>
Includes basics tests (extends fixtures.rs).
Includes doc-string documentation.
fixes cobalt-org#57
@pop
Copy link
Contributor Author

pop commented Aug 16, 2016

@johannhof I took your feedback and made the suggested changes. I had no idea that AsRef design pattern was a thing in Rust, thank's for that.

I also merged my commits for a cleaner git history, hope that's okay.

Let me know if you have any other notes.

@johannhof
Copy link
Contributor

Looks great, thanks again!

In case you're looking for more stuff to do, check out the list of missing filters (#11) 😉

@johannhof johannhof merged commit 7af117b into cobalt-org:master Aug 16, 2016
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.

Add a from_file function
2 participants