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

Can we get rid of assert to make process not fail? #140

Closed
chenyin0126 opened this issue Jun 8, 2023 · 2 comments
Closed

Can we get rid of assert to make process not fail? #140

chenyin0126 opened this issue Jun 8, 2023 · 2 comments

Comments

@chenyin0126
Copy link

There are a lot of assert keywords at repo and they make the process exit and raise an exception. I have a PDX file with one of data-object-prop has value "0.01" and type as "A_UINT32". It goes through this code

def parse_int(value: str) -> int: try: return int(value) except ValueError: v = float(value) assert v.is_integer() return int(v)

since the value is "0.01" and it raises an exception when exec " assert v.is_integer()". I agree we should do validation, but is it worth making the whole flow fail?

Maybe we can do some changes. Here are my thoughts:

Option 1: Instead of using assert, we can use the if statement to check if the value type is expected. If not expected, we can add an error description returned.

Option 2: Instead of using assert, we use an if statement, if the value is not expected, we should ignore this part of the information.

@andlaus
Copy link
Collaborator

andlaus commented Jun 9, 2023

the idea is that if one of these asserts triggers, the input file is incorrect and thus your workflow would most likely break anyway, but later and in much more subtle ways. (the issue you mention above is a good example of this, IMO. also, if you currently would remove all asserts, you would get a lot of mypy errors.) That said, there might be a few incorrect assert statements (if you find any, please open a PR), and the asserts should probably not raise AssertationError, but OdxError and use the use warnings module to make it suppressible.

I'm aware of these problems and -- if nobody beats me to this -- intend to fix them at some point, but IMO there currently are plenty of more important construction sites in the code base.

@andlaus
Copy link
Collaborator

andlaus commented Sep 5, 2023

a non-strict mode has been introduced in #168. (see [README.md] how to use it.) closing the issue as fixed for now.

@andlaus andlaus closed this as completed Sep 5, 2023
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

2 participants