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

Update exception hierarchy #30

Closed
wants to merge 1 commit into from
Closed

Update exception hierarchy #30

wants to merge 1 commit into from

Conversation

knkski
Copy link
Contributor

@knkski knkski commented Dec 7, 2021

Adds a package-level exception that FatalError and MaybeFatalError inherit from, which in turn every other exception inherits from. Also adds status field with suggested charm status.

Fixes #26

@knkski knkski requested a review from a team as a code owner December 7, 2021 18:36
@knkski knkski force-pushed the new-exception-hierarchy branch from 1c28d59 to d6e68f2 Compare December 7, 2021 18:37
@@ -17,7 +17,19 @@
]


class NoCompatibleVersions(Exception):
class SDIError(Exception):
status = BlockedStatus
Copy link
Contributor

@johnsca johnsca Dec 7, 2021

Choose a reason for hiding this comment

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

What about instead doing something like:

Suggested change
status = BlockedStatus
status_class = BlockedStatus
@property
def status(self):
return self.status_class(str(self))

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed, the other examples (OCIImageResourceError and CheckFailed) have .status as a fully instantiated status, not a recommended class. Either property or actual status is fine with me

@ca-scribner
Copy link
Contributor

ca-scribner commented Dec 7, 2021

An aside for when this gets pushed/published: lots of charms import/watch for the old errors, so we should make sure to bump SDI version to at least 0.4 as I think most of those charms are pinned to sdi<0.4.0
edit: I misread the change at first. I don't think it is backward incompatible as I expected. If it wont break things, ignore this :)

Adds a package-level exception that FatalError and MaybeFatalError
inherit from, which in turn every other exception inherits from. Also
adds `status` field with suggested charm status.
@knkski knkski force-pushed the new-exception-hierarchy branch from d6e68f2 to a431a0a Compare December 7, 2021 22:35
@beliaev-maksim beliaev-maksim deleted the new-exception-hierarchy branch June 12, 2023 21:36
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.

Refactor Exceptions to include .status
4 participants