-
Notifications
You must be signed in to change notification settings - Fork 16
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
Test insert()
and insert_all()
#41
Conversation
Co-Authored-By: Daniel Roy Greenfeld <pydanny@gmail.com>
Found 1 changed notebook. Review the changes at https://app.gitnotebooks.com/AnswerDotAI/fastlite/pull/41 |
Co-authored-by: Audrey Roy Greenfeld <arg@answer.ai>
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 fixing our tests @pydanny! I found a couple tiny things to fix here, otherwise nice work.
@@ -0,0 +1,755 @@ | |||
{ |
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.
Commented on notebook nbs/test_insert.ipynb
Cell 13 Line 1
Test None doesn't change anything. Note: The code below raises ZeroDivisionError: integer division or modulo by zero
@pydanny Remove my note if it's now returning {}
:)
@@ -50,4 +50,5 @@ | |||
'fastlite.core.create_mod': ('core.html#create_mod', 'fastlite/core.py'), |
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.
Commented on notebook fastlite/_modidx.py
Cell 1 Line 53
'fastlite.core._parse_typ': ('core.html#_parse_typ', 'fastlite/core.py'),
'fastlite.core._row': ('core.html#_row', 'fastlite/core.py'),
'fastlite.core._tnode': ('core.html#_tnode', 'fastlite/core.py'),
'fastlite.core.all_dcs': ('core.html#all_dcs', 'fastlite/core.py'),
'fastlite.core.create_mod': ('core.html#create_mod', 'fastlite/core.py'),
'fastlite.core.diagram': ('core.html#diagram', 'fastlite/core.py'),
'fastlite.core.get_typ': ('core.html#get_typ', 'fastlite/core.py')},
'fastlite.db_dc': {},
Is there a way to exclude this, since it was just for a test?
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.
I'm not sure if we should leave it in or remove it.
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.
Don't worry about _modidx (ever!)
fastlite/kw.py
Outdated
@@ -172,7 +176,8 @@ def insert( | |||
columns: Union[Dict[str, Any], Default, None]=DEFAULT, | |||
strict: opt_bool=DEFAULT, | |||
**kwargs) -> Table: | |||
if not record: record={} | |||
if not kwargs and not record: return {} |
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.
If you moved this a couple of lines down I think you can just do “if record”?
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.
That doesn't work, it breaks the tests and behavior. Perhaps we're missing a detail?
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.
After looking at it again, we addressed this, reducing line count in the method.
Co-authored-by: Audrey Roy Greenfeld <arg@answer.ai>
Co-authored-by: Audrey Roy Greenfeld <arg@answer.ai>
Thank you! |
RETURNING *
Add data returns for INSERT/UPDATE/UPSERT sqlite-minutils#42