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

A catch-all exception handler around the main file processing loop. #41

Closed
wants to merge 2 commits into from
Closed

Conversation

EvilRenegade
Copy link

This improves the situation detailed in issue #39 in so far as that exceptions no longer lead to the abortion of the entire processing batch.
Exceptions are output to stderr with stack trace and the filenames being processed, so that the underlying reason and the potentially faulty files can be investigated.

The execution of this is not pretty.
Ideally, exception handlers should be limited to the parts of the code which actually throw exceptions, and exceptions should be handled by name.
This was a quick job to stop the software from needlessly aborting the operation.

The patch was written against the current stable from pip, but applied cleanly to the current development version.

@aspiers
Copy link
Contributor

aspiers commented Sep 3, 2016

Thanks a lot! This immediately highlights another advantage of submitting a PR vs. a diff - it causes the automatic CI to run :)

sorted_messages_size)
except:
exc_type, exc_value, exc_traceback = exc_info()
logger.error("\n# # # # # # # EXCEPTION # # # # # # #\nType: %s\nValue: %s\nFiles parsed: %s\n# # # # # # # # # # # # # # # # # # #\n", exc_type, exc_value, message_files, exc_info=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is causing Travis CI to fail with:

./maildir_deduplicate/deduplicate.py:283:80: E501 line too long (202 > 79 characters)

This is easily fixable, e.g. something like

logger.error(
    "\n# # # # # # # EXCEPTION # # # # # # #\n"
    "Type: %s\nValue: %s\nFiles parsed: %s\n"
    "# # # # # # # # # # # # # # # # # # #\n", 
    exc_type, exc_value, message_files, exc_info=True)

(untested) should do it.

@aspiers
Copy link
Contributor

aspiers commented Sep 3, 2016

Uh-oh, now I might have put you in a situation you didn't want - where you need to amend a PR :-/ If you want to do it, this might help:

Otherwise feel free to give up and I can close this one and submit a new version myself :)

This improves the situation detailed in issue #39 in so far as that exceptions no longer lead to the abortion of the entire processing batch.
Exceptions are output to stderr with stack trace and the filenames being processed, so that the underlying reason and the potentially faulty files can be investigated.

The execution of this is not pretty.
Ideally, exception handlers should be limited to the parts of the code which actually throw exceptions, and exceptions should be handled by name.
This was a quick job to stop the software from needlessly aborting the operation.

The patch was written against the current stable from pip, but applied cleanly to the current development version.

----
Amended to change the formatting of the log message, because Travis CI is stuck in the 90s (80 char width limit).
I have not actually tested this change. I'm sure Travis will complain anew if the syntax is off.
@codecov-io
Copy link

codecov-io commented Sep 4, 2016

Current coverage is 61.41% (diff: 38.70%)

Merging #41 into develop will decrease coverage by 0.33%

@@            develop        #41   diff @@
==========================================
  Files             3          3          
  Lines           319        324     +5   
  Methods           0          0          
  Messages          0          0          
  Branches         71         71          
==========================================
+ Hits            197        199     +2   
- Misses           91         94     +3   
  Partials         31         31          

Powered by Codecov. Last update 7dfb841...4fe003c

…id-ass Travis won't quit crying about functionally irrelevant shit.

A warning is one thing.
Failing a build over violated style guidelines is stupid.
Copy link
Owner

@kdeldycke kdeldycke left a comment

Choose a reason for hiding this comment

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

Good catch. Code style fixes has been merged upstream in commit: 8961326 .

@kdeldycke kdeldycke added this to the 2.0.0 milestone Oct 15, 2016
@kdeldycke kdeldycke removed this from the 2.0.0 milestone Nov 11, 2016
krig added a commit to krig/maildir-deduplicate that referenced this pull request Jan 11, 2017
Some email may contain illegal characters according to the
encoding they specify.

Example error generated:

    UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 3701: ordinal not in range(128)

With this change, any sets with mail that have errors like that
are skipped and counted in a separate rejection bin.

This at least lets the deduplication process continue processing
all the mail.

Resolves: kdeldycke#39
Resolves: kdeldycke#41
krig added a commit to krig/maildir-deduplicate that referenced this pull request Jan 11, 2017
Some email may contain illegal characters according to the
encoding they specify.

Example error generated:

    UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 3701: ordinal not in range(128)

With this change, any sets with mail that have errors like that
are skipped and counted in a separate rejection bin.

This at least lets the deduplication process continue processing
all the mail.

Resolves: kdeldycke#39
Resolves: kdeldycke#41
@github-actions
Copy link

github-actions bot commented Oct 5, 2020

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 5, 2020
@kdeldycke kdeldycke added ✨ enhancement Improvement or change to an existing feature and removed enhancement labels Nov 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
✨ enhancement Improvement or change to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants