-
Notifications
You must be signed in to change notification settings - Fork 284
Extract GETOrder behavior into unit testable method #1483 #1682
Conversation
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.
Thank you for submitting this extraction. This looks great overall with some thoughts provided to improve the unit test. Would like to see more of this, but we're trying to merge two large pull requests (#1666 and #1675) which may end up conflicting heavily with changes in the core
package. Please communicate via a new issue or join our slack if you'd like to coordinate your contributions with us. We can probably get this merged quickly but might not be able to on future pull requests.
That said, if you would clean these up, I'll get this in. Thanks again!
PS: Don't worry about the coveralls CI failure. It always fails for external contributions. |
Thank you very much for the review @placer14 |
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.
GTM with one minor nitpick. If you can get to it, great. Otherwise, we'll clean it up after merge. 🤝
Thanks @ozamiatin. If you'd like to continue, here are some good next steps: #1683 |
Great, thanks a lot! Will look at #1683 |
Extracted method core.GetOrder()
Added unit test for it