Skip to content

Conversation

@jshier
Copy link
Contributor

@jshier jshier commented Oct 22, 2018

Update: This PR has been closed in favor of the revised version #20958. See also: the second review thread.

This PR adds Result<Value, Error>, as an implementation of Adding Result to the Standard Library.

This PR has basic tests at the moment, suggestions for additional tests and proper annotations are welcome.

@jrose-apple jrose-apple added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Oct 23, 2018
Copy link
Contributor

Choose a reason for hiding this comment

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

Since value and error are both calculated, would it not be more performant to have a switch on lhs and rhs and perform the equality tests there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'd be good to have a solid answer one way or another.

Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to my above comment, would it not be better to do a switch on self here? Of course one of the cases would need to add some extra bit to the hasher in order to ensure that the hashes are different when the Error type and the Value are the same and the values are equal...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. If it's more efficient, then I'll make the change, but can I annotate the properties to regain the efficiency?

Copy link
Contributor

@DevAndArtist DevAndArtist left a comment

Choose a reason for hiding this comment

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

The stdlib follows a cetrain code style which you should also follow in your PR:

  • indentation: one tab equals two spaces
  • max line width is set to 80 characters
  • If for instance a function declaration does not fit the line you should wrap it differently:
  public static func == (
    lhs: Result<Value, Error>,
    rhs: Result<Value, Error>
  ) -> Bool {

@jshier
Copy link
Contributor Author

jshier commented Oct 25, 2018

@DevAndArtist I've updated to match the style you mentioned.

@jshier
Copy link
Contributor Author

jshier commented Oct 25, 2018

I think I've created all of the API I need. Should I also do the inline documentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should follow other files to change "2017" to "2018" here.

Copy link
Contributor

@beccadax beccadax left a comment

Choose a reason for hiding this comment

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

The tests could use a little more work, but the actual implementation looks good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than making a category just for Result, why not make an "Errors" category, move ErrorType.swift and Result.swift into it, and move it above "Misc"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do whatever the team wants here. I just added a separate Result category because Optional had its own category.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to provide ~= (switch-matching) operators for Value: Equatable and Error patterns? (Hmm, technically that should be part of the proposal if it's included...)

@jshier jshier closed this Dec 3, 2018
@AnthonyLatsis AnthonyLatsis removed the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Apr 18, 2023
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.