-
Notifications
You must be signed in to change notification settings - Fork 5
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
289 feature dynamodb data accessor structure for emily api #305
289 feature dynamodb data accessor structure for emily api #305
Conversation
22fdf02
to
69da85b
Compare
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.
I'm not adding tests here because they're going to take longer than a day or so, and getting this out will unblock Setbern's work on the web interface.
I think you should add tests to this PR (or each of the smaller ones if you break it up). If you feel it's okay enough to unblock Setbern now, we could just ask him to pull down this branch and work off of that. But to merge we should add tests1.
Should this PR be broken up.
(I forget where this question was asked, just gonna respond here)
I feel like the answer is something like "not for this PR, but yes going forward". Well, this is already just a draft so I could also see myself coming out the other way on this one.
My reasoning: I feel like large PRs are quite taxing on the reviewer, much more so than a sequence of smaller ones with the same code (usually because they are packaged coherently). This usually translates into a better review where we are more likely to catch stuff. Maybe I'm alone in my feeling here, curious about what other people have to say.
Footnotes
-
I recently broke this rule in https://github.com/stacks-network/sbtc/pull/300 😞. But this was mainly because adding a test was part of another larger ticket that was about adding tests. ↩
2091d1c
to
6e96ce2
Compare
@djordon; Oh, just saw your "PR should be broken down" comment. Roger that; |
I agree with this reasoning to break it up. There are a lot of lines of code here; I thought I'd be able to get away with it because the majority of the lines of code are object definitions that are already defined in the LLD / implicitly defined by the resources, but on reflecting it doesn't really matter when reviewing because it's all content you need to look at. |
6e96ce2
to
1925c91
Compare
* feat: Change Makefile to Support integration tests for Emily. * feat: Deposit integration tests. * feat: Withdrawal integration tests. * feat: Add integration tests for chainstate + comments and tags * feat: Add missing config feature ignore to non-integration tests. * chore: Remove unused feature definition * feat: Add order comparison operators to models and sort vectors in tests
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.
Looks good. Just run cargo fmt
in the emily directory and then feel free to merge.
Use the following template to create your pull request
Description
Add database access code to the Emily API.
This PR adds the following changes:
I'm not adding tests here because they're going to take longer than a day or so, and getting this out will unblock Setbern's work on the web interface.