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

Change logging level #832

Merged
merged 3 commits into from Oct 5, 2018

Conversation

aniav
Copy link
Contributor

@aniav aniav commented Sep 11, 2018

As discussed in #527 (a long time ago) I have modified the logging level from exception to debug.

What is currently happening when using django-import-export on production is that we want to show import errors to the user and we do that in django admin, but besides from these we get loads and loads of errors being reported to Sentry, which we can't really change the configuration for without extending Resource on our end and overriding how import_data works inside. Which as a result might disable the possibility to upgrade the library.

I am proposing changing all logger.exceptions to logger.debug as basically we only need these logging details when testing our code using django-import-export and in all other cases the consuming code should be able to handle the raised exceptions in the way it needs.

I am happy to change the PR to whatever you find valuable, as long as the logger level is lower than error, so it doesn't get reported.

@coveralls
Copy link

coveralls commented Sep 11, 2018

Coverage Status

Coverage decreased (-0.03%) to 92.458% when pulling 08f75b4 on aniav:change-logging-level into 54232db on django-import-export:master.

@bmihelac
Copy link
Member

bmihelac commented Oct 2, 2018

Hi @aniav! Thanks for your PR and sorry for late reply. I see that travis reports some errors for this pull request. Can you check this?

@aniav
Copy link
Contributor Author

aniav commented Oct 4, 2018

@bmihelac ah, sorry for that 🙈 Updated now!

@bmihelac bmihelac merged commit c4c0664 into django-import-export:master Oct 5, 2018
@bmihelac
Copy link
Member

bmihelac commented Oct 5, 2018

merged, thanks @aniav

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants