-
Notifications
You must be signed in to change notification settings - Fork 6
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
Initial version of the OpenKlant2 client #1381
Conversation
ff17d65
to
01d0d7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question: why are we building the version into the package name? Is this provisional?
return cast(PaginatedResponseBody[Partij], self.process_response(response)) | ||
|
||
def retrieve( | ||
self, /, uuid: str | uuid.UUID, *, params: Optional[PartijRetrieveParams] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only method (at least in this class) where you explicitly mark positional-only arguments as such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was by design: I wanted the path parameters to be positional only, to distinguish them from the other kind of parameters. The idea is that all path parameters would then appear in sequence as positional-only args.
src/openklant2/tests/test_partij.py
Outdated
|
||
|
||
@pytest.fixture() | ||
def an_organisatie(client): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment
rekeningnummers: Required[list[ForeignKeyRef] | None] | ||
voorkeursRekeningnummer: Required[ForeignKeyRef | None] | ||
|
||
voorkeurstaal: LanguageCode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the distinction between Required
and NotRequired
is exhaustive, but this is neither (same for the attributes below)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need Required
mainly when we want to indicate "The key should be present, but it can be optionally None
", as above for e.g. voorkeursRekeningnummer
. If the field is required and not a union, no explicit Required
is needed I believe.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1381 +/- ##
===========================================
- Coverage 95.25% 94.96% -0.29%
===========================================
Files 1008 1032 +24
Lines 37370 37949 +579
===========================================
+ Hits 35595 36039 +444
- Misses 1775 1910 +135 ☔ View full report in Codecov by Sentry. |
01d0d7d
to
78de82f
Compare
The thinking here is that we already have an |
95851e1
to
8b23d82
Compare
Leaving this in draft for the moment: I have moved on to the implementation and am finding some missing params in the Partij resource, I'll add them to this PR. |
8b23d82
to
e589b57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, couple minor suggestions
e589b57
to
2a3881d
Compare
No description provided.