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 readuntil type piracy #1083

Merged
merged 3 commits into from Aug 4, 2023
Merged

Remove readuntil type piracy #1083

merged 3 commits into from Aug 4, 2023

Conversation

mortenpi
Copy link
Contributor

This makes sure that the readuntil function used by HTTP is owned by the IOExtras module, and so that it would be safe to add the currently type-pirated method taking an IOBuffer. Towards #1021.

I am not entirely sure what the best approach here is in terms of code maintainability though. Currently, I opted to keep the name the same (readuntil) and export it from IOExtras, so any module doing using IOExtras and using an unqualified readuntil will error (as the same name is exported from Base).

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2023

Codecov Report

Merging #1083 (9aaabdb) into master (10182c0) will decrease coverage by 0.03%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #1083      +/-   ##
==========================================
- Coverage   82.33%   82.30%   -0.03%     
==========================================
  Files          32       32              
  Lines        3040     3041       +1     
==========================================
  Hits         2503     2503              
- Misses        537      538       +1     
Files Changed Coverage Δ
src/IOExtras.jl 76.31% <50.00%> (-2.07%) ⬇️
src/Connections.jl 84.87% <100.00%> (ø)
src/Messages.jl 85.47% <100.00%> (ø)
src/Streams.jl 96.21% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

LGTM; I retried the failing tests, not sure if the failures are due to changes here or not.

@mortenpi
Copy link
Contributor Author

mortenpi commented Aug 3, 2023

It looks like the invalidations test failure is unrelated?

@quinnj
Copy link
Member

quinnj commented Aug 4, 2023

All looks good; thanks @mortenpi!

@quinnj quinnj merged commit bbe1746 into JuliaWeb:master Aug 4, 2023
10 of 11 checks passed
@mortenpi mortenpi deleted the mp/readuntil branch August 4, 2023 17:01
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