Skip to content

Should we rename any of the exceptions names? #1786

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

Closed
MVrachev opened this issue Jan 19, 2022 · 1 comment
Closed

Should we rename any of the exceptions names? #1786

MVrachev opened this issue Jan 19, 2022 · 1 comment

Comments

@MVrachev
Copy link
Collaborator

Description of issue or feature request:

We should make sure we review the exception names before releasing 1.0.0 and change them if necessary while it's easy.
Some of the rename suggestions are:

  1. UnsignedMetadataError -> (Metadata)SignatureError: UnsignedMetadataError suggests that there is no Signature, which is not what it is exclusively used for.
  2. BadVersionNumberError -> (Metadata)VersionError contains unnecery prefix Bad which doesn't provide any new information. If it's an error it's logical it's bad.
  3. LengthOrHashMismatchError: seems both over-explicit, and also like it's withholding information because the raiser does know if the length or the hash is the problem ... but maybe that detail is not relevant to the catcher?
    a) Split LengthOrHashMismatchError to LengthMismatchError and HashMismatchError
    b) rename LengthOrHashMismatchError to (Metadata)ContentError
  4. DownloadLengthMismatchError -> LengthError: We have a good exception message that mentions that this exception while downloading:
    image

The problem is if we rename it to LengthError it can collide with LengthMismatchError from point 3 a)

  1. SlowRetrievalError -> TimeoutError: SlowRetrievalError suggests that an attack is happing, but it could also just be a bad wire. Also, the couple of places where we actually catch an exception and throw SlowRetrievalError are variations of TimeoutError, so renaming SlowRetrievalError to a TimeoutError makes sense.

This issue documents suggestions from comments: #1725 (comment) and #1725 (comment)

@MVrachev
Copy link
Collaborator Author

Together with a couple of the maintainers @jku, @lukpueh and @joshuagl we decided that we won't make any renames for now.
Some of the renames are good but are not worth the trouble.

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

No branches or pull requests

1 participant