Implement a file reader class.

[Baryonics]

No description provided.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 76 complexity · 68 duplication

Metric	Results
Complexity	✅ 76 (≤ 100 complexity)
Duplication	68

View in Codacy

🟢 Coverage 100.00% diff coverage · +1.34% coverage variation

Metric	Results
Coverage variation	✅ +1.34% coverage variation (-0.10%)
Diff coverage	✅ 100.00% diff coverage (100.00%)

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (c5b467a)	Report Missing	Report Missing	Report Missing
Head commit (097615c)	305 (+137)	300 (+137)	98.36% (+1.34%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#19)	137	137	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

¹ Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[YanzhaoW]

Hi,

thanks for the changes. But I think some naming can be improved:

• has_one -> check_size_one
• is_end -> check_not_end_and_increment
• iterator -> chunk_iter

[YanzhaoW]

Thanks for the update.

As for the clang-tidy warning, since it's primarily from the test script, you could just disable that specific warning with:

// NOLINTBEGIN (readability-function-cognitive-complexity)

// ... code that causes the warning ...

// NOLINTEND (readability-function-cognitive-complexity)

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[YanzhaoW]

Question is whether it makes sense to continue the loop when any of these errors occurs?

[Baryonics]

I would say that it makes more sense to stop it. I pushed a commit that sets done_=true; in the error case. That way, the last read, would be the one, throwing the error.

[YanzhaoW]

Yeah, but in this case, is_done_ and has_error_ is duplicated. I believe has_error_ can never be true inside operator*() because has_error_ being true means is_done_ also being true, which in turn ends the loop.

[YanzhaoW]

Ah, Ok, you don't have has_error anymore. So maybe change done_ to is_done_.

[YanzhaoW]

Sorry, I checked the code again. Why does it current_ has to be a std::expected? It seems that it can only be valid. If it contains error state, is_done_ would be true and it's actually not being returned.

[Baryonics]

Yeah you are right, didn't thought about that. I pushed another commit. I added another bool to make sure the error gets returned.

[Baryonics]

But wait, I think I will implement a small enum class State to eliminate those bools (I think they're a bit ugly to be honest)

[YanzhaoW]

But still I don't think we should return a std::expected from operator*(). It seems the return value can never have error value and it's additional if check on the user side.

For the enum class, I think you could just have a member variable with the type ErrorCode. I added a success enum there and you can use this to show the success execution of the loop. For the unfinished status, invalid can be used here for the initial value. Then to see whether the loop has ended, you can check whether the enum is still invalid.

[Baryonics]

I pushed a commit where operator*() now returns a EntrySpan instead of std::expected. The usage is now like this

for (const auto& entry : reader)
{
    // handle entries
}
if (auto error = reader.get_status(); error  != ErrorCode::success)
{
    std::println("Error: {}",  error);
}

I thought about adding another member auto Binary::is_read_successful() -> bool{return status_ == ErrorCode::success; }

That way the API would be a bit shorter (?)

// ...
if (not reader.is_read_successful())
{
    std::println("Error: {}",  reader.get_status());
}

But this would also be quite redundant in my opinion.
What do you think?

[YanzhaoW]

yeah, it would be better. But the function name is too long. Maybe just use is_ok()?

[YanzhaoW]

This looks amazing now. I'm ready to merge it. Please fix your doxygen errors and squash everything into one commit.(If you are not confident, better to have a backup before doing the squashing).

[YanzhaoW]

Good, we could merge it.

1. Please first squash into one commit.
2. Add/edit your message on the top. Give a short summary (or a list) of what have been changed or added. Then use Github Fixes function and link this pr to the existing issue, See this.