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

fix: remove exclude_defaults #229

Merged
merged 8 commits into from
Feb 21, 2024
Merged

fix: remove exclude_defaults #229

merged 8 commits into from
Feb 21, 2024

Conversation

tstorek
Copy link
Collaborator

@tstorek tstorek commented Dec 29, 2023

closes #228

@tstorek tstorek added this to the inheritance of ContextEntity milestone Dec 29, 2023
@tstorek tstorek changed the title fix: removed exclude_defaults fix: remove exclude_defaults Dec 29, 2023
@tstorek tstorek self-assigned this Dec 29, 2023
@tstorek
Copy link
Collaborator Author

tstorek commented Dec 29, 2023

@djs0109 unfortunetaly, I cannot see, where the pipeline fails. Please, let me know, what the problem is :)

@tstorek tstorek requested a review from djs0109 December 29, 2023 19:46
try:
res = self.post(
url=url,
headers=headers,
data=subscription.model_dump_json(exclude={'id'},
exclude_unset=True,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the exclude_unset is also removed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djs0109 its because if it was excluded. The default values if not explitly set would be removed as well. Hence, the user would expect the default value will be sent to the plattform.

headers = self.headers.copy()
try:
res = self.post(
url=url,
headers=headers,
json=entity.model_dump(exclude_unset=True,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the exclude_unset is also removed here?

try:
res = self.post(
url=url,
headers=headers,
data=registration.model_dump_json(exclude={'id'},
exclude_unset=True,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the exclude_unset is also removed here?

headers=headers,
params=params,
data=query.model_dump_json(exclude_unset=True,
exclude_none=True),
Copy link
Contributor

@djs0109 djs0109 Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exclude_unset

@@ -96,9 +96,8 @@ def post_groups(self,
url = urljoin(self.base_url, 'iot/services')
headers = self.headers
data = {'services': [group.model_dump(exclude={'service', 'subservice'},
exclude_none=True,
exclude_unset=True) for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exclude_unset

@@ -115,9 +115,7 @@ def post_notification(self, notification: Message):
headers = self.headers.copy()
data = []
for entity in notification.data:
data.append(entity.model_dump(exclude_unset=True,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exclude_unset

@djs0109
Copy link
Contributor

djs0109 commented Jan 2, 2024

@tstorek I just marked all the places where the exclude_unset is also removed. Did you have some criteria for removing or not removing the exclude_unset?

@tstorek
Copy link
Collaborator Author

tstorek commented Jan 2, 2024

@djs0109 I explained above why I think that it is better to also exclude unset. This only means taht all basic filip models must use None as default, if they should not be sent by default. Sound reasonable? Do the tests work now?

@djs0109
Copy link
Contributor

djs0109 commented Jan 2, 2024

@tstorek It sounds reasonable. But for the consistency, do you think it is better to remove all exclude_unset? Btw, the tests can run through now.

@tstorek
Copy link
Collaborator Author

tstorek commented Jan 2, 2024

@djs0109 i removed them wherever it was safe at the first glance. Probably for consistency it would be good to remove them all. Please, feel free to check. :) thank you

@djs0109
Copy link
Contributor

djs0109 commented Jan 22, 2024

@tstorek I remove all exclude_unset for consistency. And I think this is actually better in the case of NGSI endpoints changing certain behaviors, such as changing the previous default value. If all tests run through, I will merge this PR. What do you think?

@djs0109 djs0109 merged commit 4b58a6b into master Feb 21, 2024
@djs0109 djs0109 deleted the 228-Requests-remove-defaults branch February 21, 2024 08:13
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

Successfully merging this pull request may close these issues.

Requests remove defaults
2 participants