topic/JANRAIN-5916/add-delimiter-to-csv #8
Conversation
Added Tests to test without a delimiter and with different delimiters
janrain_datalib/utils.py
Outdated
| Example: | ||
| >>> to_csv(['first', '2nd,with,commas', 'lasty']) | ||
| #>>> to_csv(['first', '2nd,with,commas', 'lasty']) |
There was a problem hiding this comment.
Why add a # here? Is it special syntax for Python comments?
There was a problem hiding this comment.
Had an error at one point because of that. ill fix it and re-push
janrain_datalib/utils.py
Outdated
| writer = csv.writer(output) | ||
|
|
||
| # if the delimiter is set use it | ||
| if(delim): |
There was a problem hiding this comment.
What if you had delim=',' in the function declaration? Then you wouldn't need the if statement? I don't know if that's the same as not passing it though.
If you need to leave the if statement, I'd say remove the parentheses around delim.
There was a problem hiding this comment.
I had considered this but if someone passes in None it will replace the comma and the method will error
| if delim : | ||
| writer = csv.writer(output, delimiter=delim) | ||
| # otherwise use the default comma delimiter | ||
| else: |
There was a problem hiding this comment.
You don't need this conditional, just set the default for delim to a comma
There was a problem hiding this comment.
after discussion this makes sense
setup.py
Outdated
| install_requires=[ | ||
| "janrain-python-api == 0.4.0", | ||
| "janrain-python-api", | ||
| "requests", |
There was a problem hiding this comment.
this shouldn't be neccessary - janrain-python-api has requests as a dependency
There was a problem hiding this comment.
Reverted this back to the original setup.py
setup.py
Outdated
| ], | ||
| install_requires=[ | ||
| "janrain-python-api == 0.4.0", | ||
| "janrain-python-api == 0.4.0", |
janrain_datalib/utils.py
Outdated
| return json.dumps(item, **kwargs) | ||
|
|
||
| def to_csv(row): | ||
| def to_csv(row, delim=None): |
There was a problem hiding this comment.
Why delim when you could use the whole word delimiter?
| Args: | ||
| row: list of items to be formatted as CSV | ||
| delim: the delimiter to use for the CSV output |
There was a problem hiding this comment.
change docs to reflect parameter name change
|
not sure if there's anything in jira to do for this release, but as far as code - we just increment the version, create a source dist tarball, and then submit it to pypi |
This code adds the ability to have a delimiter in the utils.to_cs() method.