Complete support for csv.Reader class signature. #232
Conversation
Additional params are not used by these method implementations.
|
@eduardostalinho since we are already doing the merge should we drop passing the |
| @@ -47,6 +47,8 @@ def discover_dialect(sample, encoding): | |||
| # Could not detect dialect, fall back to 'excel' | |||
| return unicodecsv.excel | |||
|
|
|||
| def get_dialect_parameters(params, dialect, parameter): | |||
| return params.pop(parameter, getattr(dialect, parameter)) | |||
There was a problem hiding this comment.
Any special reason for poping the parameter? Should we only get to avoid messing with them? The downside of pop would be someone trying to do something else with kwargs attributes after get_dialect_parameters only to find out the it's not there anymore.
There was a problem hiding this comment.
The reason of kwargs poping is not messing the future calls that use kwargs in the same functions.
Using *args and **kwargs is always a decision on where the code will be messed up.
|
@filipeximenes dropping the dialect will break backwards compatibility and also will not follow the Python's csv module API (since you can pass either the dialect or the parameters). |
|
LGTM |
This function pops keys instead of getting them.
|
updated @turicas @filipeximenes |
|
@turicas i think you can close this PR without merge. |
turicas
force-pushed
the
develop
branch
2 times, most recently
from
fa144f0
to
bbb2c57
Compare
This PR is an alternate to #209. It tries to get all parameters from import_from_csv kwargs falling back to its dialect parameters.
The relationship between csv.Reader
dialectand the other params is a little intrincated, so we decided to run the merge between dialect and the other params before the readers does and overwrite the user given parameters.