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

Refactor exception handling: Define custom exceptions as subclasses of a base Errors class #465

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

castanley
Copy link
Contributor

Description

This pull request refactors the exception handling in protect_archiver. The following changes were made:

  • Defined custom exceptions ProtectError, DownloadFailed, and AuthorizationFailed as subclasses of a base Errors class in errors.py.
  • Updated download_file.py to correctly import these exceptions.

Changes

  • errors.py: Defined ProtectError, DownloadFailed, and AuthorizationFailed as subclasses of Errors.
  • download_file.py: Updated import statements to use the newly defined exceptions.
  • Updated other modules to use the new exception hierarchy.

Motivation and Context

This refactor resolves the following issues:

  1. AttributeError: 'NoneType' object has no attribute 'get'

    • This error occurred when handling certain types of responses that resulted in None being accessed incorrectly.
  2. TypeError: catching classes that do not inherit from BaseException is not allowed

    • This error was caused by improperly defined custom exceptions that did not inherit from BaseException. The new hierarchy ensures that all custom exceptions are correctly defined as subclasses of Errors, which inherits from Exception.

How Has This Been Tested?

  • Verified that the exceptions are defined correctly.
  • Ensured the download_file.py and other modules import and use the exceptions without issues.
  • Rebuilt the package and tested with Poetry the functionality to confirm the changes are effective.
  • Note: Only the download function has been tested. Other functions have not been tested.

@danielfernau danielfernau force-pushed the main branch 2 times, most recently from 3bb62ae to a8feeb8 Compare September 3, 2024 12:12
@danielfernau danielfernau self-requested a review September 18, 2024 18:56
@danielfernau danielfernau self-assigned this Sep 18, 2024
@danielfernau danielfernau added the enhancement New feature or request label Sep 18, 2024
Copy link

codecov bot commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 42.85714% with 16 lines in your changes missing coverage. Please review.

Project coverage is 53.65%. Comparing base (80e3969) to head (8a0432c).
Report is 46 commits behind head on main.

Files with missing lines Patch % Lines
protect_archiver/downloader/download_file.py 20.00% 8 Missing ⚠️
protect_archiver/cli/download.py 0.00% 2 Missing ⚠️
protect_archiver/cli/events.py 0.00% 2 Missing ⚠️
protect_archiver/errors.py 80.00% 2 Missing ⚠️
protect_archiver/client/legacy.py 50.00% 1 Missing ⚠️
protect_archiver/client/unifi_os.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #465      +/-   ##
==========================================
- Coverage   56.81%   53.65%   -3.16%     
==========================================
  Files          23       24       +1     
  Lines         778      820      +42     
==========================================
- Hits          442      440       -2     
- Misses        336      380      +44     
Flag Coverage Δ
unittests 53.65% <42.85%> (-3.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielfernau danielfernau merged commit 73b2d12 into danielfernau:main Sep 18, 2024
9 of 11 checks passed
@danielfernau
Copy link
Owner

Hey @castanley - thanks a lot for your contribution to the project! I've integrated your suggestions with some additional housekeeping and tweaks for optimization.

Your changes are now merged! 🚀

Looking forward to more collaboration in the future.

Best,
Daniel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants