-
Notifications
You must be signed in to change notification settings - Fork 4
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
substrate: Add basic file utility to read all the lines from a file #87
substrate: Add basic file utility to read all the lines from a file #87
Conversation
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 is looking good - there are some stylistic items which we note below, and one query about something that seems missing, but we'll dig into this with you to understand the UTF-16 case and what's going on there
c2c87fe
to
168593d
Compare
168593d
to
7f6d339
Compare
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.
There's a couple of stylistic things to address, but this looks pretty close to mergable - nice work! ^_^
7f6d339
to
389aeeb
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
==========================================
+ Coverage 89.24% 89.27% +0.03%
==========================================
Files 47 48 +1
Lines 3516 3554 +38
Branches 688 703 +15
==========================================
+ Hits 3138 3173 +35
Misses 341 341
- Partials 37 40 +3 ☔ View full report in Codecov by Sentry. |
389aeeb
to
9198eb5
Compare
9198eb5
to
d12eac8
Compare
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.
LGTM, we'll merge this once the builds complete. Thank you for the contribution!
Hey @dragonmux,
This is a PR to add a small line reader to Substrate.
As you'll find in the attached test suite, I wasn't able to make it work with UTF-16 and wchar_t strings. Hope you can assist me in that regard.
Let me know what you think 🐱