[GPCAPIM-254]: API requests entry point #44
Conversation
…_API_requests_entry_point' into feature/GPCAPIM-254_API_requests_entry_point
|
ALB Target: |
|
ALB Target: |
|
ALB Target: |
|
ALB Target: |
|
ALB Target: |
| class BundleEntry(TypedDict): | ||
| fullUrl: str | ||
| resource: Patient | ||
|
|
||
|
|
||
| class Bundle(TypedDict): |
There was a problem hiding this comment.
Did not know you could do this with TypedDict and json.loads. Started off querying it because I'd probably normally have preferred a normal dataclass rather than a dictionary subclass (because it's not intuitive to me what it's going to do here, when you think about it - does creating a class that subclasses a dictionary mean that you can create one with normal class syntax? [Yes, it does.] If you do so can you access the created class attributes via the dictionary syntax? [Yes, it also does.]) but it does make the interaction quite nice.
I'm still slightly in two minds. This gives you nicer json interaction because you can do:
myentry = BundleEntry(json.loads(json_string))
...but like I say, it's kind of intuitive at first glance and then unintuitive when you stop to think about what it's doing.
You could also do:
@dataclass
class BundleEntry:
fullUrl: str
resource: Patient...which gives you the advantage that it's clearly understood as a normal class, but then you can't access it with dictionary syntax and you have to use star-magic to get it to play nice with json.loads: myentry = BundleEntry(**json.loads(json_string))
So, um... all these words partly as thinking aloud, partly to say I'm happy leaving it like this, it's quite nice and it's easy to read as long as you don't think about it too hard, but what do you think about a dataclass?
There was a problem hiding this comment.
Hmmm. Also, the TypedDict version will cheerfully accept an invalid entry in the json and just add it in, where the dataclass version will go bang. The TypedDict version is also entirely happy if one of the required keys is missing. I think this makes me prefer the dataclass version unless there's a specific need to be able to add in unexpected keys? It makes the class attributes in the TypedDict version no more than hints.
There was a problem hiding this comment.
Not sure what you mean by " but then you can't access it with dictionary syntax" ...
>>> from typing import TypedDict
>>> class Test(TypedDict):
... first: str
... second: int
...
>>> t = Test({'first':'foo','second': 2})
>>> t['second']
2
Ultimately,
- this is for type checking purposes
- makes it nice than
dict[str, str]everywhere - makes it explicit what the dict your passing around is
- not worried about validation yet as this is happy path only and only wanted something lightweight, quick and intuitive.
I have found dataclass ends up being a bit more awkward with json payloads
There was a problem hiding this comment.
The TypedDict you can access with dictionary syntax, the dataclass you can't (though you could implement __getitem__ I think it is and then you could).
How do you mean dataclass is more awkward with json payloads? You have to use the ** expansion, true, but that's OK?
I feel a bit like if we can get validation "free" now by using a dataclass why wouldn't we do that rather than saying oh we'll just do type checking now and leave validation for tech debt later once all the code has been written this way?
There was a problem hiding this comment.
I'm just a little twitchy about creating something that looks like it's going to enforce the existence of/passing of specific parameters, but then actually it silently doesn't - if there's a typo in your json it'll just let it go. TBH it probably doesn't matter that much for the live system, in that there won't be typos in the parameter names in the json in the live system, but it might make for oddities in testing.
| from typing import TypedDict | ||
|
|
||
|
|
||
| class HumanName(TypedDict): |
There was a problem hiding this comment.
See previous comment about TypedDict extensions
| from fhir.identifier import Identifier | ||
|
|
||
|
|
||
| class Patient(TypedDict): |
There was a problem hiding this comment.
See previous comment about TypedDict extensions
| from fhir import Parameters | ||
|
|
||
|
|
||
| class GetStructuredRecordRequest: |
There was a problem hiding this comment.
Again, is this just a stub? Also would it be better to drop Get from the front of this, given that it's a class rather than a function?
There was a problem hiding this comment.
This is an object we can pass around that will handle the parsing of the various things we will need, NHS number, header values, etc. Seemed sensible to build it at the point of entry, then the controller, various outbound clients can use its methods.
…vironment." This reverts commit f1a3fad.
|
ALB Target: |
|
ALB Target: |
|
ALB Target: |
…le getting in to their own testable methods.
|
ALB Target: |
|
ALB Target: |
|
ALB Target: |
|
3 participants
Description
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.