fixed db seed error#2014
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the seed_database function in spanner_client.py to include name and value columns when inserting seed nodes into the Spanner Node table. Corresponding unit tests in spanner_client_test.py were updated to verify these new fields. The review feedback suggests refactoring the candidates dictionary using a comprehension to reduce string redundancy and improving the test assertions by using a loop for better readability and efficiency.
| candidates = { | ||
| "StatisticalVariable": ["StatisticalVariable", ["Class"], spanner.COMMIT_TIMESTAMP], | ||
| "StatVarGroup": ["StatVarGroup", ["Class"], spanner.COMMIT_TIMESTAMP], | ||
| "StatVarObservation": ["StatVarObservation", ["Class"], spanner.COMMIT_TIMESTAMP], | ||
| "Topic": ["Topic", ["Class"], spanner.COMMIT_TIMESTAMP], | ||
| "c/g/Root": ["c/g/Root", ["StatVarGroup"], spanner.COMMIT_TIMESTAMP], | ||
| "StatisticalVariable": ["StatisticalVariable", "StatisticalVariable", "StatisticalVariable", ["Class"], spanner.COMMIT_TIMESTAMP], | ||
| "StatVarGroup": ["StatVarGroup", "StatVarGroup", "StatVarGroup", ["Class"], spanner.COMMIT_TIMESTAMP], | ||
| "StatVarObservation": ["StatVarObservation", "StatVarObservation", "StatVarObservation", ["Class"], spanner.COMMIT_TIMESTAMP], | ||
| "Topic": ["Topic", "Topic", "Topic", ["Class"], spanner.COMMIT_TIMESTAMP], | ||
| "c/g/Root": ["c/g/Root", "c/g/Root", "c/g/Root", ["StatVarGroup"], spanner.COMMIT_TIMESTAMP], | ||
| } |
There was a problem hiding this comment.
The candidates dictionary contains a lot of repeated strings. To improve maintainability and reduce redundancy, you could generate this dictionary programmatically. This would make it easier to add or modify fields in the future.
By defining the unique parts (like the types) separately and then building the full dictionary with a comprehension, the code becomes cleaner and less error-prone for future modifications.
| candidates = { | |
| "StatisticalVariable": ["StatisticalVariable", ["Class"], spanner.COMMIT_TIMESTAMP], | |
| "StatVarGroup": ["StatVarGroup", ["Class"], spanner.COMMIT_TIMESTAMP], | |
| "StatVarObservation": ["StatVarObservation", ["Class"], spanner.COMMIT_TIMESTAMP], | |
| "Topic": ["Topic", ["Class"], spanner.COMMIT_TIMESTAMP], | |
| "c/g/Root": ["c/g/Root", ["StatVarGroup"], spanner.COMMIT_TIMESTAMP], | |
| "StatisticalVariable": ["StatisticalVariable", "StatisticalVariable", "StatisticalVariable", ["Class"], spanner.COMMIT_TIMESTAMP], | |
| "StatVarGroup": ["StatVarGroup", "StatVarGroup", "StatVarGroup", ["Class"], spanner.COMMIT_TIMESTAMP], | |
| "StatVarObservation": ["StatVarObservation", "StatVarObservation", "StatVarObservation", ["Class"], spanner.COMMIT_TIMESTAMP], | |
| "Topic": ["Topic", "Topic", "Topic", ["Class"], spanner.COMMIT_TIMESTAMP], | |
| "c/g/Root": ["c/g/Root", "c/g/Root", "c/g/Root", ["StatVarGroup"], spanner.COMMIT_TIMESTAMP], | |
| } | |
| base_nodes = { | |
| "StatisticalVariable": ["Class"], | |
| "StatVarGroup": ["Class"], | |
| "StatVarObservation": ["Class"], | |
| "Topic": ["Class"], | |
| "c/g/Root": ["StatVarGroup"], | |
| } | |
| candidates = { | |
| subj: [subj, subj, subj, types, spanner.COMMIT_TIMESTAMP] | |
| for subj, types in base_nodes.items() | |
| } |
| actual_subjects = [val[0] for val in kwargs['values']] | ||
| actual_names = [val[1] for val in kwargs['values']] | ||
| actual_values = [val[2] for val in kwargs['values']] | ||
| self.assertEqual(actual_subjects, expected_subjects) | ||
| self.assertEqual(actual_names, expected_subjects) | ||
| self.assertEqual(actual_values, expected_subjects) |
There was a problem hiding this comment.
These assertions can be made more concise and robust by iterating through the values and checking the properties for each row within a loop. This makes the test easier to read and extend if more fields need to be checked against the subject_id in the future. It also avoids iterating over the list of values multiple times.
| actual_subjects = [val[0] for val in kwargs['values']] | |
| actual_names = [val[1] for val in kwargs['values']] | |
| actual_values = [val[2] for val in kwargs['values']] | |
| self.assertEqual(actual_subjects, expected_subjects) | |
| self.assertEqual(actual_names, expected_subjects) | |
| self.assertEqual(actual_values, expected_subjects) | |
| actual_subjects = [] | |
| for val in kwargs['values']: | |
| subject_id, name, value, _, _ = val | |
| self.assertEqual(name, subject_id) | |
| self.assertEqual(value, subject_id) | |
| actual_subjects.append(subject_id) | |
| self.assertEqual(actual_subjects, expected_subjects) |
This pull request updates the database seeding logic and its corresponding test to add two new fields,
nameandvalue, to the seededNodetable entries. The changes ensure that these fields are populated with appropriate values during the seeding process and that the tests verify their correctness.