-
Notifications
You must be signed in to change notification settings - Fork 1
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
Persistent storage solution #132
Conversation
…to perform DB operations
…b, tests domain, requirements
this seems like it got out of sync with |
Merge conflicts resolved |
data_dir = os.path.join(os.path.dirname(os.path.dirname(__file__)), "data") | ||
|
||
contributors_data = load_json_file(os.path.join(data_dir, "contributors.json")) | ||
events_data = load_json_file(os.path.join(data_dir, "events.json")) |
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.
don't see how this will work with the current "notified_events.json" and "posted_events.json" json files?
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.
We load the JSON and take the separate data and combine it into a dictionary which is then saved to the DB using the defined models
This can be replicated with
def test_migrate_events(test_db):
"""Test migrating events from events.json"""
data_path = os.path.join(os.path.dirname(os.path.dirname(__file__)), "data")
events_data = load_json_file(os.path.join(data_path, "events.json"))
if not events_data:
logger.info("No events data found in events.json")
assert True
return
db_service = DatabaseService(session=test_db)
result = db_service.migrate_events(events_data)
assert result >= 0
events = test_db.query(Event).all()
assert len(events) >= 0
For example
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.
a few changes I think to be made, but overall i think this is a much better/simpler approach
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.
Docs need a little clean up. There is inconsistent stylizing throughout and probably is a bit wordier than it needs to be. If you agree, can you make a separate issue for that.
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.
Docs need a little clean up. There is inconsistent stylizing throughout and probably is a bit wordier than it needs to be. If you agree, can you make a separate issue for that.
Agree it can be reduced in size. Will create an issue for this.
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.
briefly looked over again and this lgtm. lots of code changes so i didn't scrutinize everything they way i'd like to. i just want to get this merged in
Whats new
How to test
Following tests in a staging env have been run
Additional
I would like to merge this in after the scrape-props, and quorum-calc PRs have been merged. As this branch is based off of those.