search

civisanalytics / civis-python

Civis API Python Client
BSD 3-Clause "New" or "Revised" License
34 stars 26 forks source link

Print NULL for `civis sql` output with Nones instead of crashing #366

Closed aripollak closed 4 years ago

aripollak commented 4 years ago

Before this change, running civis sql with some cells being NULL results in this error:

  File "/Users/apollak/dev/civis-python/civis/cli/_cli_commands.py", line 166, in _str_table_result
    [[len(_v) for _v in _r] for _r in [cols] + str_rows])
  File "/Users/apollak/dev/civis-python/civis/cli/_cli_commands.py", line 166, in <listcomp>
    [[len(_v) for _v in _r] for _r in [cols] + str_rows])
  File "/Users/apollak/dev/civis-python/civis/cli/_cli_commands.py", line 166, in <listcomp>
    [[len(_v) for _v in _r] for _r in [cols] + str_rows])
TypeError: object of type 'NoneType' has no len()
mheilman commented 4 years ago

Could you please add a test and a changelog entry?

Should nulls be blanks instead? (From doing SELECT NULL in psql, it looks like psql uses blanks for nulls.)

aripollak commented 4 years ago

Could you please add a test and a changelog entry?

👍 to changelog entry. Do we want to start adding tests for extra CLI commands?

Should nulls be blanks instead? (From doing SELECT NULL in psql, it looks like psql uses blanks for nulls.)

🤷‍♂ Maybe it should be configurable? The Platform UI shows NULL.

mheilman commented 4 years ago

It looks like there's already a relevant test that would be pretty easy to copy/extend to check the None case.

Maybe ambiguity between "NULL" and NULL would be better than ambiguity between "" and NULL, but showing blanks for nulls would probably be quite a bit more readable. I think I'd rather follow psql's lead and use blanks since we can't italicize like in the platform web interface.

Adding an option seems like overkill at this point, but I'd be fine with it.