-
Notifications
You must be signed in to change notification settings - Fork 231
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 async element writer #635
Conversation
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## master #635 +/- ##
==========================================
- Coverage 64.43% 64.36% -0.08%
==========================================
Files 36 36
Lines 17288 17343 +55
==========================================
+ Hits 11140 11162 +22
- Misses 6148 6181 +33
Flags with carried forward coverage won't be shown. Click here to find out more.
|
src/writer/async_tokio.rs
Outdated
#[tokio::test] | ||
async fn element_writer_nested() { | ||
let mut buffer = Vec::new(); | ||
let mut writer = Writer::new_with_indent(&mut buffer, b' ', 4); |
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'm fairly certain that the only reason this works is that you're using a buffer type which also implements Write
rather than an exclusively-async type.
write_text_content
and other methods aren't implemented for ElementWriter<'a, W: AsyncWrite + Unpin>
As a sidenote if you can avoid duplicating the definition of ElementWriter
, please do.
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'm fairly certain that the only reason this works is that you're using a buffer type which also implements Write rather than an exclusively-async type.
Good catch! I'll update that
As a sidenote if you can avoid duplicating the definition of ElementWriter, please do.
AFAIK, the only way to do that would be to have an async-only implementation, then have the sync implementation be a wrapper around it. But since async is an optional feature in this crate (and perf is important), I don't think that's the best option.
If there's a better way, I'd love to know! :)
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 could remove Write
requirement from ElementWriter
struct and from impl block for attributes and put Write
or AsyncWrite + Unpin
to individual impl blocks. Then new functions should be named with _async
suffix and no need separate method to create async element writer.
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.
That's a good idea, I like that structure a lot more!
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.
Good work! Three issues:
- documentation on feature incorrectly formatted
- please use modules to grouping tests instead of common prefix
- I would prefer to use one
ElementWriter
struct with sets of methods for sync and async writing
Cargo.toml
Outdated
## | ||
## [writing events] to buffers implementing [`tokio::io::AsyncWrite`]. |
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 ##
doc comments is usual rustdoc markdown comments, which is processed by document-features
crate. So please rewrite that according to that. If you want to make hyperlink on writing events
, please add hyperlink target
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 think I maybe misunderstood the docs there so I removed that change.
src/writer/async_tokio.rs
Outdated
@@ -339,4 +484,93 @@ mod indentation_async { | |||
</paired>"# | |||
); | |||
} | |||
|
|||
#[tokio::test] | |||
async fn element_writer_empty() { |
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.
Instead on using prefix element_writer_
please put all tests i a module element_writer
:
mod element_writer {
use super::*;
use pretty_assertions::assert_eq;
#[tokio::test]
async fn empty() { ... }
...
}
src/writer/async_tokio.rs
Outdated
#[tokio::test] | ||
async fn element_writer_nested() { | ||
let mut buffer = Vec::new(); | ||
let mut writer = Writer::new_with_indent(&mut buffer, b' ', 4); |
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 could remove Write
requirement from ElementWriter
struct and from impl block for attributes and put Write
or AsyncWrite + Unpin
to individual impl blocks. Then new functions should be named with _async
suffix and no need separate method to create async element writer.
src/writer/async_tokio.rs
Outdated
@@ -57,6 +60,58 @@ impl<W: AsyncWrite + Unpin> Writer<W> { | |||
Ok(()) | |||
} | |||
|
|||
// /// Provides a simple, high-level async API for writing XML elements. |
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.
Just remove the whole commented block. You always can restore the previous version by checkouting a commit that GitHub shows in a PR
src/writer/async_tokio.rs
Outdated
/// Adds an attribute to this element. | ||
pub fn with_attribute_async<'b, I>(mut self, attr: I) -> Self | ||
where | ||
I: Into<Attribute<'b>>, | ||
{ | ||
self.start_tag.push_attribute(attr); | ||
self | ||
} | ||
|
||
/// Add additional attributes to this element using an iterator. | ||
/// | ||
/// The yielded items must be convertible to [`Attribute`] using `Into`. | ||
pub fn with_attributes_async<'b, I>(mut self, attributes: I) -> Self | ||
where | ||
I: IntoIterator, | ||
I::Item: Into<Attribute<'b>>, | ||
{ | ||
self.start_tag.extend_attributes(attributes); | ||
self | ||
} |
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 methods does not depend from writer, so remove them. Move existing with_attribute
and with_attributes
methods to the impl
block that does not restrict W
type parameter, then they will work for both sync and async writers
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.
Since with_attribute
is on the ElementWriter
, the only way to do this is by removing the Write
bound on the ElementWriter
struct (in order to support a general impl. I still made the change.
src/writer/async_tokio.rs
Outdated
#[cfg(test)] | ||
mod element_writer { |
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 supposed that this module will be inside the tests
module, but that's also OK
src/writer/async_tokio.rs
Outdated
use pretty_assertions::assert_eq; | ||
|
||
#[tokio::test] | ||
async fn empty() { |
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.
Hm... maybe move all those tests to doc comments of corresponding methods? I think, this will be useful. This one for write_empty_async
src/writer/async_tokio.rs
Outdated
} | ||
|
||
#[tokio::test] | ||
async fn text() { |
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 for write_text_content_async
src/writer/async_tokio.rs
Outdated
} | ||
|
||
#[tokio::test] | ||
async fn nested() { |
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.
And this for write_inner_content_async
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.
Nice! Only several small nits remained
src/writer/async_tokio.rs
Outdated
/// std::str::from_utf8(&buffer).unwrap(), | ||
/// r#"<paired attr1="value1" attr2="value2">text</paired>"# | ||
/// ); | ||
/// } |
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.
/// } | |
/// # } |
src/writer/async_tokio.rs
Outdated
/// std::str::from_utf8(&buffer).unwrap(), | ||
/// r#"<empty attr1="value1" attr2="value2"/>"# | ||
/// ); | ||
/// } |
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.
/// } | |
/// # } |
And also please dedent the visible code in example
src/writer/async_tokio.rs
Outdated
/// </inner> | ||
/// </outer>"# | ||
/// ); | ||
/// } |
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.
/// } | |
/// # } |
And also please dedent the visible code in example and check that indent is correct (the first visible line has different indent that the others)
src/writer/async_tokio.rs
Outdated
/// Write some text inside the current element. | ||
/// # Example | ||
/// | ||
/// ```rust |
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.
/// ```rust | |
/// ``` |
The code blocks already have rust
type by default
src/writer/async_tokio.rs
Outdated
/// Write an empty (self-closing) tag. | ||
/// # Example | ||
/// | ||
/// ```rust |
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.
/// ```rust | |
/// ``` |
The code blocks already have rust
type by default
src/writer/async_tokio.rs
Outdated
/// Create a new scope for writing XML inside the current element. | ||
/// # Example | ||
/// | ||
/// ```rust |
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.
/// ```rust | |
/// ``` |
The code blocks already have rust
type by default
Also, please add entry to
|
1cc447e
to
87bd505
Compare
87bd505
to
e2de822
Compare
Thanks for your work! |
Sometimes TortoiseGit loses commits during rebase... What a dangerous bug
Sometimes TortoiseGit loses commits during rebase... What a dangerous bug
Sometimes TortoiseGit loses commits during rebase... What a dangerous bug
This PR adds support for writing to buffers implementing tokio's
AsyncWrite
. It mirrors theElementWriter
implementation, nearly identically. The only difference is inwrite_inner_content
, the closure needs to return the Writer, I couldn't make the lifetimes line up any other way.It also includes a full test suite, again mirrored from
ElementWriter
and docs.