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 io::Write render method (i.e. render_into_bytes and render_bytes) #323

Closed

Conversation

cipriancraciun
Copy link
Contributor

As promissed in #163, here is a pul request that does the following:

  • adds a new render_into_bytes that takes a io::Write instance, and adapts it to fmt::Write and calls the plain render_into method;
  • adds a new render_bytes similar to render that returns a Vec<u8> instead of a String;

The two changes above are trivial, however there is a backward compatibility issue: in order to properly return an io::Error, I had to add the Io enum variant to Error, therefore this will fail in cases where the user hasn't included a _ pattern in the match.

Additionally, the bulk of the pull request, in order to aid tests, I've done the following:

  • created an assert_render! macro that should replace the usage of assert_eq!(t.render().unwrap(), "expected") with just assert_render!(t, "expected"); the macro calls both render() and render_bytes() to check that the output is correctly generated via both implementations;
  • I've updated all the test cases to use the assert_render! macro described above; (I've left the integration tests alone;)
  • I've updated the benchmarking code to include the render_bytes function; (I've renamed the previous function with the _string suffix, and added a new one with _bytes suffix to be more clear;)

Regarding benchmarks, it appears that the render_bytes (i.e. Vec<u8>) method is somewhat slower than the render (i.e. String) variant.

BTW, the assert_render! macro could be used in the future to make sure that for example no write methods are called with an empty string as I've reported in another issue.

@cipriancraciun
Copy link
Contributor Author

All the tests that fail are the ones related to various integrations, which also fail in the current master. The main testing tests pass.

@djc
Copy link
Owner

djc commented May 23, 2020

At least the lint failures are due to your changes. Current master also passes and was only pushed yesterday.

@cipriancraciun
Copy link
Contributor Author

@djc Could you please tell me what commands should I run locally to check the patches?

@cipriancraciun
Copy link
Contributor Author

OK, I've tested with:

cargo fmt --all -- --check
cargo clippy --all-targets -- -D warnings

And the tests have passed.

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.

This mostly looks good, thanks for working through it!

I think the test macro is overkill here, though.

askama_shared/src/io/mod.rs Outdated Show resolved Hide resolved
askama/src/lib.rs Show resolved Hide resolved
self.render_into_bytes(&mut buf)?;
Ok(buf)
}
/// Renders the template to the given `writer`. (It is recommended to use a buffered implementation.)
Copy link
Owner

Choose a reason for hiding this comment

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

The second sentence of the comment here should be on a separate line, separated from the first line by an empty line. On the first line, please remove the ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

askama/src/lib.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,28 @@

Copy link
Owner

Choose a reason for hiding this comment

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

I don't want to do this for every template, since the implementation for the io::Write adapter seems pretty simple and doesn't obviously intersect with other types of complexity in the templating engine. As such -- it would just slow down testing. We can add testing for it to a particular few test cases that you think are relevant.

As such, it doesn't seem like this macro adds much value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can decide not to include the commit with the test changes.

Also if testing speed is an issue (which in fact isn't), we could make the io::Write test case execute only in 10% of the cases, chosen randomly.

Also if you compare the code before assert_eq and after assert_render you'll notice that the code is somewhat cleaner due to the removal of the render().unwrap().

askama/src/lib.rs Outdated Show resolved Hide resolved
askama_shared/src/io/mod.rs Show resolved Hide resolved
@cipriancraciun
Copy link
Contributor Author

OK, the latest branch contains all the changes requested above, and I've squashed all commits into just three:

  • the actual code (the first one);
  • the benchmark code;
  • the assert_render! macro;

@djc
Copy link
Owner

djc commented May 24, 2020

If you remove the macro commit, I'll merge this. For testing, just add some render_into_bytes() and render_bytes() calls into one or two existing test cases.

@cipriancraciun
Copy link
Contributor Author

OK, in the latest submission, I've done the following:

  • I've removed the assert_render! macro as asked; (although I believe it was useful;)
  • I've added a single test for the render_bytes in the test_variables test case which seems to generate the most write calls;

Additionally I've also merged the generic write implementation discussed in #324, but as part of SizedTemplate as discussed.

@djc
Copy link
Owner

djc commented May 25, 2020

Okay, now please squash the changes from Add support for WriteTemplate and Merge WriteTemplate and SizedTemplate into a single commit and make sure that it presents the minimum necessary diff (that is, don't include any reordering of methods/traits in your changes).

@cipriancraciun
Copy link
Contributor Author

@djc This pull request is ready for merged.

@djc
Copy link
Owner

djc commented May 25, 2020

Sorry, but the squashed commit still doesn't satisfy my review as mentioned in my previous comment.

@cipriancraciun
Copy link
Contributor Author

Sorry, but the squashed commit still doesn't satisfy my review as mentioned in my previous comment.

Which is what? I had to move the SizedTemplate code into where Template was, because else it would have meant moving the write_into function from there into SizedTemplate which would have created an even bigger patch...


Really, I'm starting to think that perhaps the best course for me is just forking this and patching it there... I don't mind keeping with some "guidelines", but so far at least 50% of my time was spent pleasing the cargo fmt, cargo clippy and trying to argue for some sane options...

So, this is the pull request in its final format.

@djc
Copy link
Owner

djc commented May 25, 2020

Okay, we'll close this too, then.

Note that this isn't fun for me, either. I've spent quite a bit of time responding to all of your messages and in a number of cases you've kept rebuffing my guidelines for whether/how I wanted to accept patches into this repository. Please remember that, while these might be one-off contributions for you, I will be on the hook to maintain these features for the long term, and as such I want Askama to adhere closely to the design I have in mind.

If you have such strong ideas about how a templating engine should work, forking might indeed be the best solution. If you do fork, please remember to provide correct attribution per the applicable license.

@djc djc closed this May 25, 2020
@cipriancraciun
Copy link
Contributor Author

Please remember that, while these might be one-off contributions for you, I will be on the hook to maintain these features for the long term, and as such I want Askama to adhere closely to the design I have in mind.

Usually for my own needs I just fork and patch the library myself. However because I do plan to use Askama for a professional project, for long term, I am too invested in Askama, given that it is the only mature compile-time Rust template engine.

Therefore my efforts in the last few days to patch some issues that I've found in my initial run with Askama.

I do value your time, and I'm grateful that you've built Askama; however the time of other contributors (even minor ones) should be valued the same.

For example if one of the patches isn't fully to your liking, especially in the areas of formatting or moving code from one place to another, I think it's easier to just checkout the pull request, make the required changes and merge that... All this back-and-forth for minor issues is tiresome...

@djc
Copy link
Owner

djc commented May 25, 2020

I do value your time, and I'm grateful that you've built Askama; however the time of other contributors (even minor ones) should be valued the same.

I'm afraid I disagree. While I value your time, I value mine more -- and as the maintainer, I think this is fair, as explained earlier. (And even if it wasn't, it's my prerogative.)

I do often give first-time contributors a pass on some of the details of how best to contribute, but you have persisted in going a different direction than I suggested I was open to several times, which makes me less inclined to make an extra effort towards your contribution. You've also asked me to spend substantial time on features that I consider of low value (like workarounds for your idiosyncratic code style).

Also note that I do my reviews and merging/rebasing pretty much exclusively through GitHub's web UI, so squashing some commits or reordering code would involve pulling your branches first, then doing rebases. Even if our time were of equal value, it would be more efficient for you to do these things.

I have substantial experience as a maintainer of open source projects and I've found it more frustrating to engage with you compared to most other contributors who've participated in my projects.

@hybras
Copy link

hybras commented May 23, 2021

This thread got a little heated. @djc ru open to this work being continued? I'm still familiarizing myself with the project and this pr, but I think I'll have time in a week. I will probably need advice from you unless the remaining work is trivial.

@djc
Copy link
Owner

djc commented May 23, 2021

@hybras yeah, definitely still open to this work being continued. I'll have to swap the context back in, but feel free to ask me any questions (probably best to do it in the context of an issue or a new PR). Probably useful to familiarize yourself with the discussion in this PR, too.

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.

None yet

3 participants