Remove asserts from client library code#201
Conversation
mikix
left a comment
mikix
left a comment
There was a problem hiding this comment.
I totally agree that this library could use a healthy injection of type checking. Easiest to start with the custom code. But also fhir-parser could benefit from it, and then have that bleed into this repo's models.
Also agreed that those loopholes would be good to close.
| - Alexandru Stanciu <https://github.com/ducu> | ||
| - Alexander Gorokhov <https://github.com/sashgorokhov> |
There was a problem hiding this comment.
This is such a tiny thing, but this isn't the right alphabetical order, right? You should be at the top there.
| assert self.base_uri and path | ||
| if not path: | ||
| raise ValueError(f"Parameter `path` must not be None or empty, got: {path}") |
There was a problem hiding this comment.
You dropped the check for base_uri.
| self.server = FHIRServer(self, state=state.get("server")) | ||
| base_uri = self.server.base_uri if self.server is not None else None | ||
| self.server = FHIRServer( | ||
| client=self, | ||
| base_uri=state.get('server', {}).get('base_uri') or base_uri, | ||
| state=state.get("server") | ||
| ) |
There was a problem hiding this comment.
OK so now we get into an actual behavior change 😄 You are trying to pass on the existing base_uri as a default value. I can see some sense to that, but it might be surprising that some of the old state config would survive a call to from_state. I guess it's just a fallback? So without this code, it wouldn't work anyway. But maybe that's a clearer behavior? Rather than a surprise base_uri being snuck in if you don't expect it? Did you hit the error code path yourself?
As for the code changes, assuming we do like this behavior change:
- ignorable taste nit: You could rewrite that
base_uri =line asbase_uri = self.server and self.server.base_uri. (a) FHIRServer doesn't define__bool__or__len__, so a simple truthy check is equivalent tois not Noneand scans better, (b) I find the "X and Y" chaining easier to read than the ternary "Y if X else None" operator. - actual probably should change it nit: You don't need to dig into the state dict for it's
base_uri-FHIRServer.from_statewill already do that for you when its init method calls it. You can just pass inbase_uri=base_uriand letfrom_statehandle the rest.
There was a problem hiding this comment.
And if we keep this change, probably worth a small unit test to confirm behavior.
There was a problem hiding this comment.
Hell, from a coverage perspective, the ValueErrors also deserve a unit test. Though that's another todo item - enforce coverage.
2 participants
Using asserts in production code can be dangerous in cases where python interpreter is running with -O optimization option, which will remove all assertion statements completely from bytecode.
There multiple ways how to address this, one simplest would be is to replace assert statement with if condition that would raise an exception, making check logically equivalent.
Originally I wanted to refactor logic to make these checks unnecessary at all, but that resulted in lots of changes that snowballed out of control. I've found couple of logical loopholes that allow creating instances with broken state. Being first time contributor in this project I decided to start small first.
Speaking of loopholes, there are several:
__init__, and does not check what is provided into.from_stateTo address all these, I would honestly start with adding typing into the whole library, and then start adjusting any logic responsible for instance initialization and correct-state-insurance into from_state method. "state" and "from_state" can operate on TypedDict instances (ok to use since this library is python >=3.10), and through them, library can communicate which attributes are required or not.
What do you guys think? I'd be excited to help y'all with these improvements.