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

Add specific exceptions for String issues #155

Merged
merged 4 commits into from
Jun 29, 2019
Merged

Conversation

ashafer01
Copy link
Contributor

My application can expect either raw bytes or a string in the same underlying ASN1 components. The current exception hierarchy makes this situation basically impossible to handle.

I have added new exceptions specific to string encoding and decoding, and utilized multiple inheritance to ensure that users catching any relevant type of error will still catch these errors. i.e. users that have updated their code to catch PyAsn1Error in this situation can continue doing so unaffected, and users that were already catching the Unicode*Errors and have not updated will be fixed with this patch.

Also worth noting that Python .encode() and .decode() will always use the relevant Unicode*Error regardless of the passed encoding.

@codecov-io
Copy link

codecov-io commented Mar 24, 2019

Codecov Report

Merging #155 into master will decrease coverage by 0.04%.
The diff coverage is 26.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #155      +/-   ##
==========================================
- Coverage   85.64%   85.59%   -0.05%     
==========================================
  Files          29       29              
  Lines        4076     4083       +7     
==========================================
+ Hits         3491     3495       +4     
- Misses        585      588       +3
Impacted Files Coverage Δ
pyasn1/type/univ.py 83.58% <12.5%> (ø) ⬆️
pyasn1/type/char.py 92.12% <12.5%> (ø) ⬆️
pyasn1/error.py 70% <57.14%> (-30%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25cf116...1b5e7b8. Read the comment docs.

@etingof
Copy link
Owner

etingof commented Jun 23, 2019

Sorry for long delay...

Alternatively, would it make sense to let unicode-related exceptions to bubble up as-is? I fair that duplicating system exceptions is going to become messy very soon...

@ashafer01
Copy link
Contributor Author

ashafer01 commented Jun 23, 2019

Yeah I think letting them bubble would be much cleaner and more maintainable, but I wanted to provide a compatible solution since this behavior has changed over various releases.

With that in mind, I think for the spots where LookupError is also handled it would be better and more compatible to throw as a PyAsn1Error as before, but always just re-raise (or just dont catch) UnicodeErrors.

@etingof
Copy link
Owner

etingof commented Jun 29, 2019

Contemplating this matter more, I think your initial solution makes more sense for now. In the next, less compatible, revisions we could just let UnicodeError to bubble up...

Thank you!

@etingof etingof merged commit fe2725f into etingof:master Jun 29, 2019
@etingof
Copy link
Owner

etingof commented Jun 29, 2019

JFYI: I've renamed the exception classes in the follow up patch in hope to make it more obvious to the users.

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.

None yet

3 participants