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

Added NoError type #132

Merged
merged 1 commit into from
Jan 29, 2016
Merged

Added NoError type #132

merged 1 commit into from
Jan 29, 2016

Conversation

NachoSoto
Copy link
Contributor

Fixes #130

@gfontenot
Copy link
Member

Is it weird to hide this away at the bottom of Result.swift?

This seems to be a general pattern we're using so it's probably not worth holding up this PR for. I do wonder if we shouldn't do a pass at better file organization though.

@gfontenot
Copy link
Member

Anyway, I dig the change. Good stuff.

@JaviSoto
Copy link
Contributor

Is it worth exposing an extension on Result where Error == NoError with a var that returns a non-optional T value?

Other than that 👍 this is great!

@NachoSoto
Copy link
Contributor Author

Is it weird to hide this away at the bottom of Result.swift?

Good point, I didn't consider that. It's definitely more important than that! I'd consider how it looks when looking at the generated "header" Swift module though.

@NachoSoto
Copy link
Contributor Author

Is it worth exposing an extension on Result where Error == NoError with a var that returns a non-optional T value?

Ooooh good idea!

@robrix
Copy link
Contributor

robrix commented Dec 15, 2015

Lovely ❤️

@NachoSoto
Copy link
Contributor Author

What do people think of the 2 questions?

  • Location: @gfontenot where would you recommend placing it instead?
  • non-optional value in an extension with where Error == NoError?

@gfontenot
Copy link
Member

I don't know that I have a better recommendation right now (other than maybe just a NoError.swift file). I'm OK with not changing anything here and instead doing a larger audit on the organization later.

@gfontenot
Copy link
Member

Oh, and I love the idea of a non-optional value property. Should it be named something else to avoid ambiguity?

@NachoSoto
Copy link
Contributor Author

It can probably share the same name (because of lvalue type inference), but it would mean that some users would have to explicitly specify the type, so maybe we should provide a different name?

I filed this radar (dupes appreciated! maybe I should submit to swift-evaluation?) precisely for this reason. Ideally the optional type would not exist for NoError.

@mdiep
Copy link
Contributor

mdiep commented Jan 29, 2016

I'm going to go ahead and merge this as-is. I don't think the location is terribly important (and can be changed later) and Swift doesn't make a non-optional variant very easy.

mdiep added a commit that referenced this pull request Jan 29, 2016
@mdiep mdiep merged commit d20dd5c into antitypical:master Jan 29, 2016
@NachoSoto NachoSoto deleted the no-error branch January 29, 2016 22:14
@NachoSoto
Copy link
Contributor Author

😍

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.

5 participants