-
Notifications
You must be signed in to change notification settings - Fork 34
Allow schema-enforced input CSVs, general refactor #27
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
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.
LVGTM, added some small changes to enable installing test dependencies via the test requirements files. Even though we're at 90% coverage I believe that specific test files ( even if we have time for it ) will help us both understanding what each file does, and to fully cover them.
WDYT? :)
$ pytest --cov redisgraph_bulk_loader/
========================================================================================= test session starts =========================================================================================
platform linux -- Python 3.6.9, pytest-5.3.5, py-1.8.1, pluggy-0.13.1
rootdir: /home/filipe/redislabs/redisgraph-bulk-loader
plugins: cov-2.8.1
collected 12 items
test/test_bulk_loader.py ............ [100%]
----------- coverage: platform linux, python 3.6.9-final-0 -----------
Name Stmts Miss Cover
-------------------------------------------------------------
redisgraph_bulk_loader/__init__.py 1 0 100%
redisgraph_bulk_loader/bulk_insert.py 76 12 84%
redisgraph_bulk_loader/config.py 19 0 100%
redisgraph_bulk_loader/entity_file.py 151 19 87%
redisgraph_bulk_loader/exceptions.py 4 0 100%
redisgraph_bulk_loader/label.py 58 5 91%
redisgraph_bulk_loader/query_buffer.py 41 0 100%
redisgraph_bulk_loader/relation_type.py 66 7 89%
-------------------------------------------------------------
TOTAL 416 43 90%
@@ -0,0 +1,31 @@ | |||
class Config: |
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 but lets add a test to this class ( not dependant on redisgraph ). It will help on bug tracking on further iterations
|
||
|
||
# Handler class for processing relation csv files. | ||
class RelationType(EntityFile): |
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.
same as above. missing specific test file
from config import Config | ||
|
||
|
||
class QueryBuffer: |
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.
same as above. missing specific test file
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 class only pushes binary data to Redis, I'm not sure how I would test it.
from exceptions import SchemaError | ||
|
||
|
||
class Label(EntityFile): |
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.
same as above. missing specific test file
|
||
|
||
# Superclass for label and relation CSV files | ||
class EntityFile(object): |
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.
same as above. missing specific test file
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.
Since this class is only a superclass and not invoked directly, I don't feel that it needs separate tests.
from exceptions import CSVError, SchemaError | ||
|
||
|
||
class Type(Enum): |
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.
should we move it to a specific entity_type class file?
same as above. missing specific test file
fac4945
to
69e8579
Compare
Codecov Report
@@ Coverage Diff @@
## master #27 +/- ##
=========================================
Coverage ? 89.62%
=========================================
Files ? 8
Lines ? 405
Branches ? 0
=========================================
Hits ? 363
Misses ? 42
Partials ? 0 Continue to review full report at Codecov.
|
Contains the enhancements proposed in #20 without any breakage of backwards compatibility (with the exception of replacing the
--fields
argument with schema-enforced headers).Enhancements include: