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

Rewrite link_checker to use a Result internally #928

Merged
merged 1 commit into from
Apr 22, 2020
Merged

Conversation

awonnacott
Copy link
Contributor

@awonnacott awonnacott commented Jan 20, 2020

link_checker::LinkResult always had exactly one member option set, so it should be an enum, not a struct. Instead of writing our own enum to represent sucess/failure, we can use std::result::Result.

@Keats
Copy link
Collaborator

Keats commented Jan 22, 2020

What's the benefit if we are not using ??

@awonnacott
Copy link
Contributor Author

It's not about using Result in particular, it's about how the data you are representing (EITHER code OR error) should be represented with an enum, whereas a struct "means" code AND error. If you use a struct to store two values and only one of them is set, you

  • open up a class of errors which are impossible with an enum: using an uninitialized value or forgetting to check if error is set
  • waste space, since your struct takes up sizeof(code) + sizeof(error) memory instead of max(sizeof(code), sizeof(error)).

Using types whose value sets correspond directly to the desired representable values is one of the big advantages of functional programming, and having well-designed enums is among the advantages Rust has over Go or C++. Consider how useful it is that existing Rust libraries return Results instead of Go-style pairs of value and error.

@Keats
Copy link
Collaborator

Keats commented Feb 11, 2020

Can you cherry-pick your commit on top of next and push again?

@awonnacott
Copy link
Contributor Author

@Keats done

@awonnacott
Copy link
Contributor Author

hmm, tests passed on my machine, will investigate

@Keats
Copy link
Collaborator

Keats commented Mar 25, 2020

Errors seem legit

@Keats Keats merged commit d19855e into getzola:next Apr 22, 2020
@Keats
Copy link
Collaborator

Keats commented Apr 22, 2020

I'll fix the error on the next branch directly; thanks!

@awonnacott awonnacott deleted the next branch April 25, 2020 02:47
@awonnacott awonnacott restored the next branch April 25, 2020 02:47
@awonnacott awonnacott deleted the next branch February 2, 2024 19: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.

2 participants