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

Remove the ability to use un-buffered functions with Reader<&[u8]> #440

Merged
merged 2 commits into from
Jul 25, 2022

Conversation

dralley
Copy link
Collaborator

@dralley dralley commented Jul 24, 2022

No description provided.

@codecov-commenter
Copy link

Codecov Report

Merging #440 (b75fbd3) into master (6d883b5) will increase coverage by 0.11%.
The diff coverage is 65.38%.

@@            Coverage Diff             @@
##           master     #440      +/-   ##
==========================================
+ Coverage   51.15%   51.27%   +0.11%     
==========================================
  Files          27       27              
  Lines       13314    13289      -25     
==========================================
+ Hits         6811     6814       +3     
+ Misses       6503     6475      -28     
Flag Coverage Δ
unittests 51.27% <65.38%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
benches/microbenches.rs 0.00% <0.00%> (ø)
src/events/mod.rs 68.60% <ø> (ø)
src/reader/mod.rs 90.60% <ø> (-0.31%) ⬇️
src/de/mod.rs 74.50% <93.33%> (-0.67%) ⬇️
src/reader/buffered_reader.rs 75.51% <100.00%> (+9.82%) ⬆️
src/reader/ns_reader.rs 60.37% <100.00%> (ø)
src/reader/slice_reader.rs 100.00% <100.00%> (ø)
src/lib.rs 12.26% <0.00%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d883b5...b75fbd3. Read the comment docs.

Copy link
Collaborator

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

Approved, only remove mentioned functions -- their name assumes that thay can borrow, but that's not true anymore, so I think there's no need to keep them. We have a from_reader for that

src/de/mod.rs Outdated Show resolved Hide resolved
src/reader/ns_reader.rs Outdated Show resolved Hide resolved
In the near future, decoding will be performed automatically as the
input is read. If the input has an unknown encoding, it must be decoded
first, necessitating a buffer. Therefore only the buffered
implementation can be used for `Reader::from_bytes()`
@dralley dralley merged commit 8ab2dbe into tafia:master Jul 25, 2022
@dralley dralley deleted the split-readers branch July 25, 2022 14:29
Comment on lines -299 to -307
/// Deserialize an instance of type `T` from bytes of XML text.
pub fn from_slice<'de, T>(s: &'de [u8]) -> Result<T, DeError>
where
T: Deserialize<'de>,
{
let mut de = Deserializer::from_slice(s);
T::deserialize(&mut de)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still, the constructors were probably deleted in vain. What do they interfere with? It's still may be valuable to pass a byte array and allow quick_xml to detect encoding and parse it. If encoding is UTF-8, then borrowing is possible. @dralley, what do you think?

If it remains removed, then it is needed to fix dangling documentation links

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

re: documention, whoops, yes, I can fix that

re: constructors, IMO it is unnecessary, this is why we have the decoding functions as separate now. It's only an extra line or two of code and if we made a constructor which included that line it would need to be falliable and return Result, unlike the others.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should make the CI fail on documentation issues if possible ^^

Mingun added a commit that referenced this pull request Jul 25, 2022
Mingun added a commit to Mingun/quick-xml that referenced this pull request Jul 28, 2022
One is a dangling reference from tafia#440

The others fixed by converting interlink to HTML link, because rustdoc
doesn't understand interlinks to realized trait methods
dralley pushed a commit that referenced this pull request Jul 28, 2022
One is a dangling reference from #440

The others fixed by converting interlink to HTML link, because rustdoc
doesn't understand interlinks to realized trait methods
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.

3 participants