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
Add support for io::Write
render method (i.e. render_into_bytes
and render_bytes
)
#323
Conversation
All the tests that fail are the ones related to various integrations, which also fail in the current master. The main |
At least the lint failures are due to your changes. Current master also passes and was only pushed yesterday. |
@djc Could you please tell me what commands should I run locally to check the patches? |
OK, I've tested with:
And the tests have passed. |
There was a problem hiding this 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/src/lib.rs
Outdated
self.render_into_bytes(&mut buf)?; | ||
Ok(buf) | ||
} | ||
/// Renders the template to the given `writer`. (It is recommended to use a buffered implementation.) |
There was a problem hiding this comment.
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 .
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
testing/tests/assert.in
Outdated
@@ -0,0 +1,28 @@ | |||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
.
01c7036
to
6be44eb
Compare
OK, the latest branch contains all the changes requested above, and I've squashed all commits into just three:
|
If you remove the macro commit, I'll merge this. For testing, just add some |
70f5782
to
3e11a6b
Compare
OK, in the latest submission, I've done the following:
Additionally I've also merged the generic write implementation discussed in #324, but as part of |
Okay, now please squash the changes from |
…r*` family of methods it terms of this trait.
3e11a6b
to
334b1ea
Compare
@djc This pull request is ready for merged. |
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 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 So, this is the pull request in its final format. |
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. |
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... |
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. |
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. |
@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. |
As promissed in #163, here is a pul request that does the following:
render_into_bytes
that takes aio::Write
instance, and adapts it tofmt::Write
and calls the plainrender_into
method;render_bytes
similar torender
that returns aVec<u8>
instead of aString
;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 theIo
enum variant toError
, 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:
assert_render!
macro that should replace the usage ofassert_eq!(t.render().unwrap(), "expected")
with justassert_render!(t, "expected")
; the macro calls bothrender()
andrender_bytes()
to check that the output is correctly generated via both implementations;assert_render!
macro described above; (I've left the integration tests alone;)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 therender
(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.