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

reader: Dont return option for file #44

Merged
merged 2 commits into from
Oct 12, 2023
Merged

Conversation

rukai
Copy link
Contributor

@rukai rukai commented Oct 12, 2023

@Daniil-Golikov here is some extra feedback for #41 I figured it would be easiest to just demonstrate what I mean with a PR.

In general, we should never silently do nothing when we are expected to do something.
If we cannot carry out a task then we need to report to the user in some way that the task failed.
This is useful for both:

  • the final end user - they need to know what went wrong so they can try again with better input.
  • us as developers - if something goes wrong in our program it would be great if it told us what it was without a possibly lengthy debugging session.

In this case we have the partially implemented functionality around reading raw files.
Instead of returning a None, we should make some kind of error occur:

  • Since we are in early stages of fft-compression project and there are no users yet our best option is a todo!() since that will direct developers directly to the line that needs further work via the stacktrace.
  • If we did have users then we should be bubbling up an error or emitting an error! log instead. We already return a Result so we could just return a Result::Err instead of a Option::None.

Avoiding the option is also beneficial because we dont want the read_file API to be affected by the in progress nature of its current implementation. Since once its completed it wouldnt need the Option at all.

Copy link
Contributor

@Daniil-Golikov Daniil-Golikov left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback on #41.

I agree about the importance of clear error handling.
Using todo!() at this stage makes sense.
Shifting from Option::None to Result::Err for read_file is a good suggestion.

@cjrolo cjrolo merged commit bba5ded into main Oct 12, 2023
0 of 2 checks passed
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