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

Do away with non-bang #create_XXX methods? #30

Open
krallin opened this issue Jun 6, 2016 · 1 comment
Open

Do away with non-bang #create_XXX methods? #30

krallin opened this issue Jun 6, 2016 · 1 comment

Comments

@krallin
Copy link
Contributor

krallin commented Jun 6, 2016

The current #create_XXX methods (e.g. app.create_operation) swallow errors, and return a Aptible::Resource::Base object, which can be very confusing, especially considering that attempting to reload those resources will throw an immensely cryptic Resource server root URL must be defined by subclass error.

Now, we do have equivalent bang methods (e.g. app.create_operation!), which do throw exception, but they're not used everywhere.

For example, in aptible-integration, we're using app.create_operation practically everywhere), and when it started throwing Resource server root URL must be defined by subclass (https://travis-ci.com/aptible/aptible-integration/jobs/41749266), it wasn't immediately obvious at all that the cause was in API (ultimately https://github.com/aptible/api.aptible.com/pull/373).

Is there value in having methods that swallow errors? Is there value in that being the most natural method call? I think the answer to both questions is no, and I think we should simply make both methods do the same thing (i.e. raise exceptions), and possibly raise a deprecation warning on the bang methods that they're now redundant.

Thoughts @blakepettersson @fancyremarker?

@fancyremarker
Copy link
Member

I'm fine with this. The existing behavior is intended to provide some similarity to the ActiveRecord API, but the issues you point out are reason enough not to support non-bang methods.

krallin added a commit to krallin/aptible-cli that referenced this issue Jun 9, 2016
krallin added a commit to krallin/aptible-cli that referenced this issue Jun 9, 2016
krallin added a commit to krallin/aptible-cli that referenced this issue Jun 15, 2016
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

No branches or pull requests

2 participants