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

Plaid::LongTailInstitution #3

Merged
merged 1 commit into from
May 5, 2016
Merged

Plaid::LongTailInstitution #3

merged 1 commit into from
May 5, 2016

Conversation

be9
Copy link
Owner

@be9 be9 commented May 3, 2016

Implement long tail institution-related API. Everything is in the Plaid::LongTailInstitution class.

Also Plaid::Institution.get was renamed to Plaid::Institution.get! to stress that it raises an exception if FI was not found. The Plaid::LongTailInstitution.get returns nil in this case. This resembles ActiveRecord again, with its find_by and find_by! methods.

@be9
Copy link
Owner Author

be9 commented May 3, 2016

@rbuckheit be sure to check this!

@@ -77,9 +77,116 @@ def self.all
#
# Returns an Institution instance or raises Plaid::NotFoundError if
# institution with given id is not found.
def self.get(id)
def self.get!(id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't see any mutation here, why do we have the !?

Copy link
Owner Author

Choose a reason for hiding this comment

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

! also means "throws exceptions instead of returning nil". But I decided to ditch this. Most of the gem methods throw exceptions in case of error, we'd need to mark them all with !.

@rbuckheit
Copy link
Collaborator

just had one question about the use of get! which does not appear to introduce state mutation, let's address that before merging. otherwise looks great!

@be9 be9 merged commit 27c2569 into master May 5, 2016
@be9 be9 deleted the longtail branch May 5, 2016 11:12
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