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

Feature/add raw line on error tuple option #95

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

y86
Copy link

@y86 y86 commented Jun 30, 2020

Includes an option to add the raw csv line to the the error tuple and to the exceptions.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.02%) to 95.977% when pulling 3e6ae1d on y86:feature/add-raw-line-on-error-tuple-option into 2cc26de on beatrichartz:master.

@coveralls
Copy link

coveralls commented Jun 30, 2020

Coverage Status

Coverage decreased (-3.8%) to 96.154% when pulling e0750bb on y86:feature/add-raw-line-on-error-tuple-option into 87c30b9 on beatrichartz:master.

@beatrichartz
Copy link
Owner

Thanks for putting this together I'll hopefully get a look later this week

escape_sequence: escape_sequence,
line: index + 1,
escape_max_lines: escape_max_lines
).message, raw_line}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is breaking backwards compatibility if I interpret it correctly. If someone has existing code that depends on the length of the tuple being returned, they will get a compilation error. We should either try to avoid it by adding the raw line to the message, or reserve this for a major version upgrade.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I included the :raw_line_on_error option (defauls to false) so that only when set to true the extra raw_line will return on the error tuple, so I'd say it won't break backwards compatibility.

@beatrichartz
Copy link
Owner

Hey @y86 this is looking good, thanks! I think adding more information on errors is going to help users of this library get to the bottom of problems more quickly.

There are some issues with backwards compatibility in the pr as it is now. Also, there is some missing coverage of how errors get pulled through from the decoder to the top level module (CSV).

Is the idea that this should help with error messaging and human intervention, or do you use this to intervene programatically in case of errors? Depending on what the goal is, we could then ideate on possible backwards compatible ways of achieving the same thing.

@y86
Copy link
Author

y86 commented Sep 19, 2020

Hey @beatrichartz , I am not too familiar with coveralls, so I wouldn't know where to start on the tests to bring it back from where it was.
Regarding backward compatibility, as I mentioned on my reply to your comment, I think it is preserved.

Our use case for this was to easily return the problematic lines to the user in the interface so he could either fix them there and resubmit or download them into separate smaller file for review - the original csv files are quite large and sometimes they can't even edit them on Excel.

@beatrichartz
Copy link
Owner

I think this functionality could still make it into a future release but would need updating. It's become a bit more challenging due to the parser accepting byte streams additionally to line by line input. Effectively we'll need to collect the full line, which the parser is already doing unless the byte size of the input is smaller than the line length, in which case it collects only the field.

Will have a look at adapting this hopefully soon.

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.

3 participants