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

Throwing errors instead of fatalError #15

Closed
ilyapuchka opened this issue Nov 24, 2015 · 8 comments
Closed

Throwing errors instead of fatalError #15

ilyapuchka opened this issue Nov 24, 2015 · 8 comments

Comments

@ilyapuchka
Copy link
Collaborator

Probably we should consider throwing errors in resolve instead of using fatalError. For now we use it only in one place, but there could be more places where we will need to signal developer that something is wrong. try enforces developer to pay attention to possible errors. Also with throwing errors we can test those invalid cases what we can't do with fatalError.

@AliSoftware
Copy link
Owner

Yeah I was wondering about that the first time I added fatalError.

I didn't want to use throw at that stage because:

  • that would make the API call more complex, especially we couldn't let ws = container.resolve() as WebServiceAPI and would have to wrap that in a do…catch
  • In any case when the resolution fail, that's a developer error, so fatalError feels right.

If we don't do…catch in the case of the first bullet point but use try! instead, that's kinda acknowledging that failing to resolve should make the app crash and that would be a developer error not a user error, and would result in the same as doing fatalError after all.

So that was my reasoning at the time. Maybe moving to throw could make sense but if that results to all users using try! all the time, not sure that's worth it? I mean, I hate force-unwrapping optionals, so generally I also hate seeing try! in the user code as well. But maybe we should to make the developer aware of the risks, I'm not sure what I prefer here.

@ilyapuchka
Copy link
Collaborator Author

Agree that this will make to use try! with every resolve, but that's good for developer, I think, cause it indicates that this is a point where something can go wrong. So developer should think upfront is it safe to use. With fatalError he will notice that he did something wrong only at runtime. Also (just imagining, didn't thought through) there can be use cases for something like plugin systems or something backed up by in-app purchases, when not having a registered component does not mean a fatal error. The other important argument for throwing is that we will be able to test such negative cases in Dip unit test.

@AliSoftware
Copy link
Owner

Ok, you convinced me migrating to throw and let the user call try! might be indeed better.
Note that as this will break the existing API (new API not retro-compatible) this will lead to a new major version of Dip so might integrate that feature at the same time we remove the deprecated method register(instance:) and do other major changes to the API if any.

@ilyapuchka
Copy link
Collaborator Author

Maybe we can have some kind of roadmap that we discuss upfront and have somewhere in this repo which will outline a set of features we plan to release? Also we can use it to discuss do we ever need particular features or not and where will they go - in core or extensions.

@AliSoftware
Copy link
Owner

Yeah we should.
Idk what's best a github issue or a wiki that we could edit live?

I also like to use GitHub milestones to prioritize issues and tell in which version they are plan to go like a RoadMap, and use GitHub labels to tag issues (like we could have a "Core" and "Extensions" labels)

@ilyapuchka
Copy link
Collaborator Author

Wiki page will be ok I think

@AliSoftware
Copy link
Owner

Well actually I think about this:

  • using GitHub Milestones to set priorities in issues (like I do in SwiftGen)
  • using labels to mark "Core" or "Dedicated Repo"
  • using wiki to discuss this and write it as text

You should have all the necessary access right to start all this (milestones, labels, wiki) on your own if you want (I have to go tonight) otherwise will look at it tomorrow evening.

@AliSoftware
Copy link
Owner

@ilyapuchka I've created this wiki page for a start, and also the two Core and Dedicated Repo labels. Feel free to create issues for the future features you plan to integrate (like UI extensions, etc) and flag them, and to create GitHub milestones too.

@ilyapuchka ilyapuchka added this to the 4.0.0 milestone Nov 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants