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 __init__.py #12

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

elschnorro77
Copy link

fix _write_sentence for pyhton3
fix send_command for pyhton3
add missing elements from nmea sentences
add initialisation of PA1010D
add waitforfix

fix _write_sentence for pyhton3
fix send_command for pyhton3
add missung elements from nmea sentences
add initialisation of PA1010D
add waitforfix
This reverts commit 76c47e1.
@Gadgetoid
Copy link
Member

The missing NMEA sentences are handy, thank you, but I have a few concerns;

  1. Is the fix to _write_sentence just swallowing IOErrors?
  2. This is a lot of logging/error swallowing, what are you trying to achieve here and why?

This level of exception handling is a bit heavy-handed for the underlying library. If you need to catch exceptions in read/write that could be done in the client app.

Generally:

  1. Your try/catch should be around one or two lines of relevant code- as it stands the change of indentation across multiple lines of code makes it near impossible to tell what's actually changed
  2. It's redundant to catch an exception and just raise it again
  3. It's messy to catch an exception and log it at the library level, makes development iteration more difficult. If a user is concerned with logging they can add that over the top. Maybe a hardened example is the best way to handle this?
  4. If IO Errors are an intermittent problem in your setup (sometimes it's unavoidable because i2c is the devil) it might be worth having some kind of retry-on-fail, specially for writes

If you'd be happy to strip out all the exception handling for now and make this a clean PR just adding the missing NMEA sentences, we can go from there with some kind of logging example.

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.

2 participants