Use consistent naming for event constructors #431
Conversation
I think it would be OK to leave
We should say something like
|
src/events/mod.rs
Outdated
| }, | ||
| decoder: Decoder::utf8(), | ||
| } | ||
| pub fn from_unescaped(content: &'a str) -> Self { |
There was a problem hiding this comment.
Oh, now I see the question.
Yes, this is backwards from what I think would be most intuitive. IMO, the default should be to escape the input in new(), with an alternative to provide escaped input if you already have it (or if you know that it doesn't need to be escaped). It should be easier to do what is most likely correct (perform escaping) than what is most likely incorrect (not doing so).
This is the model I had in mind when writing the comment above.
There was a problem hiding this comment.
Yes, I had the same doubts. I've changed names: from_escaped and new
Codecov Report
@@ Coverage Diff @@
## master #431 +/- ##
==========================================
- Coverage 49.51% 49.32% -0.19%
==========================================
Files 22 22
Lines 13847 13805 -42
==========================================
- Hits 6856 6810 -46
- Misses 6991 6995 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
src/events/mod.rs
Outdated
| /// Creates a new `BytesStart` from the given content (name + attributes) | ||
| /// | ||
| /// Owns its contents. | ||
| /// `&content[..name_len]` is not checked to be a valid name |
There was a problem hiding this comment.
The attributes are also not checked to be valid.
There was a problem hiding this comment.
To be honest I'm not sure I see the use case for this function for the majority of users, ironically the one thing which it does get used for a lot is creating events from invalid data for the tests.
I'm not saying that we should remove it, but perhaps it can be discouraged more actively?
There was a problem hiding this comment.
There was a problem hiding this comment.
Sure, sounds fine. Let's still do the documentation part though.
There was a problem hiding this comment.
Could you write a suggestion?
There was a problem hiding this comment.
How about
&content[..name_len]must be a valid name, and the remainder ofcontentmust be correctly-formed attributes. Neither are checked, it is possible to generate invalid XML ifcontentorname_lenare incorrect.
There was a problem hiding this comment.
Done + added similar warnings to all other constructors where there were none
|
Approved: but please allow #425 to be merged first |
|
Alright, well, go ahead and fix up the conflicts here and on the other PR. Might as well go ahead and merge them. |
…tead As a result, it became clear that the `test_read_write_roundtrip_escape` test did not check anything, so it was deleted
…caped` Writer is anywhere works only in UTF-8, so this constructor is not needed to be public
Writer is anywhere works only in UTF-8, so this constructor is not needed to be public
…ng instead of &[u8] / Vec<u8>
…BytesStart::new` Replace `BytesStart::owned` and `BytesStart::borrowed` by `BytesStart::from_content`
Replace `BytesText::from_plain_str` by `BytesText::new`
There is an agreement that constructors should be at the beginning
Co-authored-by: Daniel Alley <dalley@redhat.com>
Continuation of #428 (includes it changes) that unifies all event constructor names. I'm not sure about
BytesText::new, however. While this name is very aligned and consistent with other names, I afraid, it can be error-prone. Would it be better to usenewor leavefrom_escaped?