-
Notifications
You must be signed in to change notification settings - Fork 449
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
refactor: database schema code #3968
Conversation
Signed-off-by: Meet Soni <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3968 +/- ##
==========================================
+ Coverage 75.41% 80.44% +5.03%
==========================================
Files 808 820 +12
Lines 11983 12567 +584
Branches 1598 1950 +352
==========================================
+ Hits 9037 10110 +1073
+ Misses 2593 2042 -551
- Partials 353 415 +62
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Hm, so self.table_schemas()[1]
definitely looks nicer, but I don't think it really adds a lot of clarity and readability. We'd really want it to be something like self.table_schemas["range"]
or maybe self.table_schemas.range
probably switching it to a dictionary (the first of those options) will make it easier to iterate in some of the other code. I'd suggest that you get rid of the table_schemas() function and instead make a nice class data structure that can be used directly.
Might want to think about whether we can make a data structure that makes it easier to do #3965 too. They're not quite using the same data but close.
Sure, I'll look into it. |
for #3965:
I don't think any other structural changes will help. |
In general, I don't mind putting # nosec on bandit stuff when there's no good alternative -- it adds a bit of release overhead for me because I review all the nosec lines at that point, but if we provide sufficient explanation in the comments it's not a big deal to note them and move on. But that said, i think what you've got above is a nice solution that pleases bandit and makes the whole thing a little easier to understand, so let's go with that. |
removed table_schemas() function and added top level dictionary to directly access the database tables with table names Signed-off-by: Meet Soni <[email protected]>
added dictionary to be used for better table name validation. This will help resolve bandit issues in intel#3965. Signed-off-by: Meet Soni <[email protected]>
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! Let's get this merged.
Closes: #3117.
cc @terriko @anthonyharrison