-
Notifications
You must be signed in to change notification settings - Fork 0
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
♻️ Slight changes to get_client and to_internal_data #47
Conversation
stevenbal
commented
Nov 28, 2024
- add option to raise exceptions for get_client
- no longer make assertions on the response of to_internal_data, because it could be a 204 (empty) response
* add option to raise exceptions for get_client * no longer make assertions on the response of to_internal_data, because it could be a 204 (empty) response
3dc8777
to
78a8324
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
==========================================
+ Coverage 54.95% 55.43% +0.48%
==========================================
Files 79 80 +1
Lines 3632 3604 -28
Branches 590 472 -118
==========================================
+ Hits 1996 1998 +2
+ Misses 1533 1503 -30
Partials 103 103 ☔ View full report in Codecov by Sentry. |
@@ -183,6 +183,9 @@ def _test_nrc_config(check_autorisaties_subscription=True) -> list: | |||
nrc_config = NotificationsConfig.get_solo() | |||
nrc_client: Optional[Client] = NotificationsConfig.get_client() | |||
|
|||
if not nrc_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.
Although it might be nice to return early here, looking at the code it currently returns this code whenever no client was configured. Unless we want to have different behavior I think we should return that here too or remove this path which returns early.
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 added this because I saw the change here: open-zaak/open-zaak@08c7ccd
Though I'm not sure if we need it at all?
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.
discussed on slack: replaced it with the current behavior in OZ
cf329e4
to
83d4d60
Compare