Skip to content
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

Cursors to optionally return dict with column names #235

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

darren-martz
Copy link

Optionally have one or all cursors return results as a dictionary with the column names as keys.

set every cursor to return dict by default

my_connection.as_dict = True

set one cursor to return dict by default

my_cursor.as_dict = True
for row in my_cursor:
    print(row["mycolumn"])

@codecov-io
Copy link

codecov-io commented Dec 11, 2019

Codecov Report

Merging #235 into master will decrease coverage by 0.72%.
The diff coverage is 55.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
- Coverage   99.04%   98.31%   -0.73%     
==========================================
  Files         145      145              
  Lines        3135     3385     +250     
==========================================
+ Hits         3105     3328     +223     
- Misses         30       57      +27
Impacted Files Coverage Δ
python/turbodbc/cursor.py 94.22% <47.61%> (-4.8%) ⬇️
python/turbodbc/connection.py 95.83% <75%> (-4.17%) ⬇️
cpp/turbodbc_numpy/Library/src/unicode_column.cpp 86.66% <0%> (-13.34%) ⬇️
cpp/turbodbc/Library/src/cursor.cpp 97.05% <0%> (-2.95%) ⬇️
...pp/turbodbc_arrow/Library/src/arrow_result_set.cpp 96.74% <0%> (-1.78%) ⬇️
...bc_python/Library/src/determine_parameter_type.cpp 96.66% <0%> (-1.64%) ⬇️
.../turbodbc_python/Library/src/python_result_set.cpp 93.87% <0%> (-1.58%) ⬇️
...urbodbc_arrow/Library/src/set_arrow_parameters.cpp 94.8% <0%> (-1.46%) ⬇️
...urbodbc_numpy/Library/src/set_numpy_parameters.cpp 99.47% <0%> (-0.53%) ⬇️
...pp/turbodbc/Library/src/result_sets/result_set.cpp 100% <0%> (ø) ⬆️
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 137d9de...304cf4a. Read the comment docs.

@xhochy
Copy link
Collaborator

xhochy commented Dec 14, 2019

According to https://www.python.org/dev/peps/pep-0249/#frequently-asked-questions this API was omitted for a reason, we should thus also omit it here, too.

@darren-martz
Copy link
Author

According to https://www.python.org/dev/peps/pep-0249/#frequently-asked-questions this API was omitted for a reason, we should thus also omit it here, too.

Unfortunate situation of lowest common denominator. Does it make a difference the default is off and this is optional?

@xhochy
Copy link
Collaborator

xhochy commented Dec 15, 2019

I would rather provide a separate API that returns the dicts. What about for row in my_cursor.iterrow_dicts() ? And fetchonedict?

@darren-martz
Copy link
Author

That can work too, I’ll adjust to whatever is preferred.

One library I used called pymysql allowed the choice of output types over the same APIs. In turbodbc’s case, they would have something like an enum property such as as_type instead of as_dict, then allowed values such as None, Dict, Numpy, Arrow. Just another approach.

Which is preferred?

@xhochy
Copy link
Collaborator

xhochy commented Dec 15, 2019

I prefer APIs were the types don't depend on the internal state of an object, i.e. having different methods that for each return type. @fjetter @MathMagique feel free to weigh in here.

@fjetter
Copy link
Collaborator

fjetter commented Jan 7, 2020

I prefer APIs were the types don't depend on the internal state of an object, i.e. having different methods that for each return type.

I feel the same way. In particular we're exercising the principle already for arrow and numpy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants