-
Notifications
You must be signed in to change notification settings - Fork 184
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
Update Database on project #158
Conversation
stdevMac
commented
May 18, 2022
- Add transaction database (oriented to transactions)
- Minor changes on names and comments
Codecov Report
@@ Coverage Diff @@
## main #158 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 34 36 +2
Lines 3690 3762 +72
=========================================
+ Hits 3690 3762 +72
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 but just a couple of issues:
- The
db
package is becoming increasingly specialised to Juno's requirements so I think we should have this moved to theinternal
in the root directory of the repository. - Although not documented, the file name convention is all lowercase or lowercase with underscores. I have a preference for the former as
_xxx.go
prefixes tend to be used as directives on how the toolchain interacts with these files i.e._test.go
,_linux.go
,_arm.go
, and so on. (See issue). - It is not clear to me why is the
abi
,state
, andtransaction
packages coupled with the database. If they are related to how such objects are handled by the database then they should be under aninternal
directory underdb
which is also makes point 1. more pertinent. Shouldn't we also apply similar treatment to blocks as well? So instead ofblockSpecific
we could haveblock
and treat that similarly tostate
andtransaction
? - Using separators i.e.
268_435_456
and providing more context on the values that set the geometry might improve readability. - Do we need a separate
db_test
package?
About 3. : |
Abou 3.: |
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.
LGTM.
* update database * Remove comments in db * Add coverage * Update coverage * solve error compiling and more coverage * Move db from pkg to internal * Rename files to snake case format * Remove db_test package * Fix imports and renames of the databases constructors Co-authored-by: stdevAdrianPaez <[email protected]>