-
Notifications
You must be signed in to change notification settings - Fork 147
Remove sqlitedict from requirements and import guard #460
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
Conversation
chtruong814
left a comment
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.
Thanks for doing this! Had some minor comments.
| print( | ||
| "Eval harness with sqlitedict is deprecated. Sqlitedict has known vulnerability GHSA-g4r7-86gm-pgqc" | ||
| ) | ||
| HAS_SQLITEDICT = True |
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.
This should be false.
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.
Good catch!
| os.makedirs(os.path.dirname(cache_db), exist_ok=True) | ||
| self.dbdict = SqliteDict(cache_db, autocommit=True) | ||
| if HAS_SQLITEDICT: | ||
| self.dbdict = SqliteDict(cache_db, autocommit=True) |
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 honestly probably won't matter but probably best to set self.dbdict to None sqlite dict does not exist. It looks like in the above CacheHook it's a noopt if dbdict is None.
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.
Good suggestion, I've introduced an else statement
dimapihtar
left a comment
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. Thank you!
No description provided.