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

Resolves #365 without waiting for 2.0 #406

Closed
wants to merge 7 commits into from

Conversation

@bmaggard
Copy link
Contributor

commented Jan 14, 2017

Resolves #365 without waiting for 2.0

@alimanfoo
Copy link
Collaborator

left a comment

Looks good, just a question regarding overriding config.

@@ -297,7 +298,7 @@ def __init__(self, source, converters=None, failonerror=False,
self.converters = dict([(i, v) for i, v in enumerate(converters)])
else:
raise ArgumentError('unexpected converters: %r' % converters)
self.failonerror = failonerror
self.failonerror = failonerror or config.failonerror

This comment has been minimized.

Copy link
@alimanfoo

alimanfoo Jan 16, 2017

Collaborator

Not sure this is right. Shouldn't the user-provided value override the config? I.e., maybe the default keyword argument should be failonerror=None then you could have:

if failonerror is None:
    self.failonerror = config.failonerror
else:
    self.failonerror = failonerror

?

This comment has been minimized.

Copy link
@bmaggard

bmaggard Jan 16, 2017

Author Contributor

Agreed. That's a much better way. I shall incorporate it right away

This comment has been minimized.

Copy link
@bmaggard

bmaggard Jan 16, 2017

Author Contributor

Are you OK with this style?

self.failonerror = (config.failonerror if failonerror is None
                else failonerror)

This comment has been minimized.

Copy link
@alimanfoo

alimanfoo Jan 16, 2017

Collaborator

Fine by me.

@@ -70,7 +71,7 @@ def __init__(self, source, mappings=None, failonerror=False,
self.mappings = OrderedDict()
else:
self.mappings = mappings
self.failonerror = failonerror
self.failonerror = failonerror or config.failonerror

This comment has been minimized.

Copy link
@alimanfoo

alimanfoo Jan 16, 2017

Collaborator

Same comment as above, need to consider how user can override the config value, regardless of what the config value is.

@@ -195,7 +198,7 @@ def __init__(self, source, rowmapper, header, failonerror=False):
self.source = source
self.rowmapper = rowmapper
self.header = header
self.failonerror = failonerror
self.failonerror = failonerror or config.failonerror

This comment has been minimized.

Copy link
@alimanfoo

alimanfoo Jan 16, 2017

Collaborator

Same comment re overriding config.

@@ -283,7 +288,7 @@ def __init__(self, source, rowgenerator, header, failonerror=False):
self.source = source
self.rowgenerator = rowgenerator
self.header = header
self.failonerror = failonerror
self.failonerror = failonerror or config.failonerror

This comment has been minimized.

Copy link
@alimanfoo

alimanfoo Jan 16, 2017

Collaborator

Same comment re overriding config.

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Jan 16, 2017

Thanks for the docs! Looks good to me. Good to go?

@bmaggard

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2017

My only remaining reservation is with the value 'yield_exceptions'. I thought that 'return_exceptions' might be better, but since you didn't question it, I'm assuming that it sounds fine. If it is, then I'm done.

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Jan 17, 2017

@bmaggard

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2017

Sounds good. Some suggestions: "inline", "return"/"returned", "yield"/"yielded"

@rsyring

This comment has been minimized.

Copy link

commented Sep 7, 2018

Just wondering if there is anything holding this up. Thanks.

@bmaggard

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Sep 7, 2018

@rsyring

This comment has been minimized.

Copy link

commented Sep 11, 2018

@alimanfoo Based on the PR, it looks like it would be an improvement. I haven't pulled the branch to test it but I can do that if you want me to.

If it was me, I'd consider releasing 2.0 for the sole purpose of flipping the default on this to throw the exception. IMO, swallowing errors is just a really bad thing to do and as a dev, I'd rather know an error was there and deal with it than have things silently working outside my expectations. From a backwards compatibility perspective, with this PR change, going back to the old behavior if someone wanted that would be as simple as global config variable change. Then again, I might toy with the idea of not allowing that and just force people to handle the exceptions. But, my libraries don't typically have very widespread usage and I don't have to deal with very many angry "customers". :)

@rsyring

This comment has been minimized.

Copy link

commented Sep 11, 2018

Since you mention it, I could live with 'yield_exceptions' and couldn't immediately think of anything better, but did wonder if it might be worth a bit of discussion.

I also thought that wasn't very intuitive but couldn't immediately come up with anything better. Of the options @bmaggard provided, I kind of like "inline."

Then again, maybe the difficulty of naming is because handling the exceptions that way is just not intuitive. Maybe we remove the "inline/yield" option and give a warning option instead. That would allow a dev to turn on the warnings and see all the exceptions (assuming some are different from the first one that would be thrown) without having to investigate the dataset to find them.

The warning option would use Python's built in warning system.

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Sep 11, 2018

Thanks @rsyring for the perspective, much appreciated.

Personally I'd be happy with flipping the default to failonerror=True, I think it's a safer default for most users.

I prefer "inline" also.

I like the warnings idea, but with big tables this can generate a lot of warnings, which can be awkward (e.g., if running in a jupyter notebook, lots of output can kill the browser). Having the exceptions inline allows you to programmatically investigate which rows are generating exceptions, which can be convenient.

I propose to change "yield_exceptions" to "inline", and change the default failonerror to True, then merge. Any objections?

@bmaggard

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

No objections.

@rsyring

This comment has been minimized.

Copy link

commented Sep 11, 2018

No objections from me either. Thanks @alimanfoo for getting this merged and @bmaggard for doing the work initially.

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Sep 11, 2018

Thanks @bmaggard and @rsyring.

Brad would you be able to make the proposed changes? Also looks like a rebase or merge from master is needed to resolve a conflict?

@bmaggard

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

@bmaggard

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Sep 11, 2018

Do you want the default to change with this PR?

I would say yes, might as well. If we're going to do it, let's do it now. Some extra documentation will be needed probably both in docstrings and in release notes to make users aware of the change. If you have time & inclination to work on documentation in this PR then great, but also happy to raise an issue to add documentation and address that via a future PR if that would be easier.

@bmaggard

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Sep 11, 2018

@bmaggard

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2018

I don't see any actions I can take on this right now. Does someone need to reject/refuse the PR?

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Sep 12, 2018

Brad if you wouldn't mind making the change from "yield_exceptions" to "inline", and also rebasing your PR on upstream master, I (or @timbledum) can merge.

Btw you'd be welcome as a core dev, which would mean you could merge PRs.

@bmaggard

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2018

Rebased in #460 .

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Sep 21, 2018

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Aug 6, 2019

Closing in favour of #460.

@alimanfoo alimanfoo closed this Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.