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

NMEASentence/__getattr__: Graceful fail #143

Closed
wants to merge 1 commit into from

Conversation

barbacbd
Copy link
Contributor

** Return None and handle the error without raising an error. Technically
raising an error after catching another is bad as it still creates a stack
trace (causing a hard failure) and it demolishes the original stack. Here the
return None is handled and a message is logged in debug format. The other way
to resolve this on a hard failure is to reraise the origin after logging just call
raise to preserve the stack.

** Altered the test that utilized this code.

Fix for #94

** Return None and handle the error without raising an error. Technically
raising an error after catching another is bad as it still creates a stack
trace (causing a hard failure) and it demolishes the original stack. Here the
return None is handled and a message is logged in debug format. The other way
to resolve this on a hard failure is to reraise the origin after logging just call
raise to preserve the stack.

** Altered the test that utilized this code.
@coveralls
Copy link

coveralls commented Jul 11, 2022

Coverage Status

Coverage increased (+0.005%) to 98.348% when pulling e55386d on barbacbd:getattr_graceful_failout into a0d60b6 on Knio:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 98.348% when pulling e55386d on barbacbd:getattr_graceful_failout into a0d60b6 on Knio:master.

@barbacbd
Copy link
Contributor Author

/retest python 3.7

@barbacbd
Copy link
Contributor Author

@Knio not sure if these tests can be rerun, but it didn't look like it. The python3.7 job is failing for a reason that doesn't appear to be related to the software. Can we rerun this to confirm?

@Knio
Copy link
Owner

Knio commented Jul 28, 2022

I don't think this is a good idea to silently fail for fields that were never defined, and goes against the normal Python behavior of every other object.

It also doesn't solve #94, since that's accessing defined fields and not taking this code path, and will still fail on parsing None.

I think 94 should be solved manually in the DateTimeFix mixin.

Python3+ also properly supports raising an exception from within an exception handler, and linking it to the original exception and stack.

@Knio Knio closed this Jul 28, 2022
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.

3 participants