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

Drop Snap7Exception where reasonable #272

Closed
gijzelaerr opened this issue Apr 22, 2021 · 5 comments
Closed

Drop Snap7Exception where reasonable #272

gijzelaerr opened this issue Apr 22, 2021 · 5 comments
Milestone

Comments

@gijzelaerr
Copy link
Owner

gijzelaerr commented Apr 22, 2021

I think we should rely less on Snap7Exception, and replace it with common Python exceptions.

For example, if a user supplies the wrong type to a function we should raise a TypeError.

https://docs.python.org/3/library/exceptions.html

We should only raise a Snap7Exception if there really is a snap7 specific error, that can not be represented with buildin exceptions.

@gijzelaerr gijzelaerr changed the title Drop Snap7Exception Drop Snap7Exception where reasonable Apr 22, 2021
@gijzelaerr gijzelaerr added this to the 2.0 milestone Apr 22, 2021
@swamper123
Copy link
Contributor

I guess the only left Snap7Exception would be here:

def check_error(code: int, context: str = "client") -> None:

Since this is the main interface to get informations from the underneath snap7?

@lautarodapin
Copy link
Contributor

I guess the only left Snap7Exception would be here:

def check_error(code: int, context: str = "client") -> None:

Since this is the main interface to get informations from the underneath snap7?

Is there anyreason to keep using the informations from the snap7 lib? Having different exceptions in python will be nice for handling errors during development with python-snap7.

@swamper123
Copy link
Contributor

Our interface with snap7 is pretty small for thrown errors and dealing with thrown errors from snap7 in Python is horrible if it "just happens". It is not like a nice "ValueError" is thrown, in case stuff underneath exploded, they are much more ugly.
This check_error is a nice way to seperate our lib with the snap7 one. Also we are able to understand what was thrown from snap7 and could track down problems in an easy way.

Discussable would be creating new Exceptions/add messages to typical python ones like ValueError and use these...but I believe using builtin ones is enough for our usecases

@gijzelaerr
Copy link
Owner Author

there are a couple

λ  grep -R "raise Snap7Exception" snap7
snap7/common.py:            raise Snap7Exception(msg)
snap7/common.py:        raise Snap7Exception(error)
snap7/client.py:            raise Snap7Exception(f"The cpu state ({state.value}) is invalid")
snap7/client.py:            raise Snap7Exception("The blocktype parameter was invalid")
snap7/client.py:            raise Snap7Exception("The blocktype parameter was invalid")
snap7/client.py:            raise Snap7Exception("The parameter was invalid")
snap7/client.py:            raise Snap7Exception("The parameter was invalid")
snap7/client.py:            raise Snap7Exception("The blocktype parameter was invalid")
snap7/util.py:                raise Snap7Exception("Max size could not be determinate. re.search() returned None")
snap7/util.py:                raise Snap7Exception("Max size could not be determinate. re.search() returned None")
snap7/partner.py:            raise Snap7Exception("The Client parameter was invalid")
snap7/logo.py:            raise Snap7Exception("The parameter was invalid")
snap7/logo.py:            raise Snap7Exception("The parameter was invalid")

@gijzelaerr
Copy link
Owner Author

@lautarodapin i think we should keep all information available to us, that helps debugging problems. Just the class can be a bit more specific but also more Python std based.

@gijzelaerr gijzelaerr modified the milestones: 2.0, 1.2 Jul 7, 2021
@gijzelaerr gijzelaerr modified the milestones: 1.2, 1.3 Oct 20, 2021
@gijzelaerr gijzelaerr mentioned this issue Jan 20, 2022
swamper123 pushed a commit to swamper123/python-snap7 that referenced this issue Mar 17, 2022
@gijzelaerr gijzelaerr modified the milestones: 1.3, 1.2 May 25, 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

No branches or pull requests

3 participants