Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1168 +/- ##
==========================================
- Coverage 92.90% 92.89% -0.01%
==========================================
Files 41 41
Lines 9865 9901 +36
Branches 2013 2024 +11
==========================================
+ Hits 9165 9198 +33
- Misses 430 431 +1
- Partials 270 272 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| def setUp(self): | ||
| self.test_dir = tempfile.mkdtemp() | ||
| if os.path.exists(self.test_dir): # start clean |
There was a problem hiding this comment.
This code raised hidden errors during test setup. tempfile.mkdtemp() creates a clean temp directory so the below code is not needed
|
|
||
| def tearDown(self): | ||
| if os.path.exists(self.ns_filename): | ||
| if hasattr(self, 'ns_filename') and os.path.exists(self.ns_filename): |
There was a problem hiding this comment.
This code raised hidden errors during test setup when this test was skipped because ROS3 is not installed or there is no internet, because the instance vars are not defined before skipping and tearing down.
|
#1293 should be merged before this |
| If the object is being constructed from a file, raise a warning instead to ensure invalid data can still be | ||
| read. | ||
| """ | ||
| if table and len(data) <= self.MAX_ROWS_TO_VALIDATE_INIT: |
There was a problem hiding this comment.
MAX_ROWS_TO_VALIDATE_INIT implied to me that only that many rows would be validated, but the logic here implies that validation is fully being disabled once we have more than MAX_ROWS_TO_VALIDATE_INIT rows. I.e., I think we should either:
- Use a boolean flag on
__init__andtableto saydisable_index_validation, or - Update the logic here to only validate the first N rows as specified by
MAX_ROWS_TO_VALIDATE_INIT
I think option 1 is probably cleaner and more useful. I don't think it is very useful to validate the first 1000 rows and then miss the error in row 1001. I think for a user it is probably more intuitive to either validate or not.
There was a problem hiding this comment.
I agree and made the changes for option 1.
| t = popargs('table', kwargs) | ||
| table, validate_data = popargs('table', 'validate_data', kwargs) | ||
| data = getargs('data', kwargs) | ||
| self._validate_data = validate_data |
There was a problem hiding this comment.
Not critical. With this setup it may be nice to add a setter/getter for the validate_date field. But that can be a separate issue and is probably not necessary in most cases. Maybe something that can wait until a user actually has a real need.
2 participants
Motivation
Fix #210
Checklist
CHANGELOG.mdwith your changes?