-
Notifications
You must be signed in to change notification settings - Fork 59
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
exploding va_rx gem into vets-api -- with minor refactor #88
Conversation
|
||
# FIXME: Add this spec | ||
describe Common::Base do | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you forget to do this or are you going to add it in another pr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be in a separate PR. Currently have close to 100% coverage, but it's based off of Prescriptions. I think it should be more generic than that, but didn't want to spend umpteen hours rewriting specs when it's already at 100%
Other than @lihanli 's comment LGTM |
👍 |
def <=>(other) | ||
return -1 if prescription_id < other.prescription_id | ||
return 1 if prescription_id > other.prescription_id | ||
0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be shortened to prescription_id <=> other.prescription_id
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe... (it should) let me test that out really quick -- will make similar change for tracking if its the case.
No description provided.