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.

Sign up for GitHub

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

Jump to bottom

Fix/default value #255

Merged
merged 19 commits into from
Mar 11, 2024
Merged

Fix/default value #255

merged 19 commits into from
Mar 11, 2024

Conversation

AMontagu
Copy link
Collaborator

@AMontagu AMontagu commented Jan 4, 2024

FIX: #171

@AMontagu AMontagu requested a review from legau January 4, 2024 15:42
@markdoerr
Copy link
Contributor

Merci @AMontagu and @legau,
for solving this issue - looking forward when it is solved (if I manage, I could test it with the DSG example project).
I have a lot of issues on the LARAsuite side with it (and the proposed workaround did not work with datatime fields).

@markdoerr markdoerr mentioned this pull request Jan 5, 2024
Open
@markdoerr
Copy link
Contributor

@AMontagu and @legau,

I created a working example of this pull request - and it worked very smoothly :)

socotecio/django-socio-grpc-example#16

Great job !

legau
legau reviewed Jan 8, 2024
View reviewed changes
django_socio_grpc/mixins.py Outdated Show resolved Hide resolved
django_socio_grpc/protobuf/json_format.py
def message_to_dict(message, **kwargs):
"""
Converts a protobuf message to a dictionary.
Uses the default `google.protobuf.json_format.MessageToDict` function.
Adds None values for optional fields that are not set.
"""

kwargs.setdefault("including_default_value_fields", 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 remove this ? I would guess it breaks non optional fields with value=default

docs/features/proto-serializers.rst Outdated Show resolved Hide resolved
django_socio_grpc/proto_serializers.py Show resolved Hide resolved
django_socio_grpc/proto_serializers.py Outdated
if field.field_name in data_dict:
continue
# INFO - AM - 04/01/2024 - Adding default None value only for optional field that are required and allowing null or having a default value
if field.allow_null or field.default in [None, empty] and field.required is True:
Copy link
Contributor

Choose a reason for hiding this comment

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

When does field.default in [None, empty] and field.required is True happen ?
In _get_cardinality comes from not field.required or field.default not in [None, empty]

@AMontagu
Copy link
Collaborator Author

AMontagu commented Jan 8, 2024

@markdoerr we have an issue as specified by Leni. With new method when user want to update a non optional field to the default grpc value (0 for int or "" for string) it does not update as removed from the gRPC message.

Will work on that on friday but @legau told me it may not have a solution without specific user declaration.

@AMontagu
Copy link
Collaborator Author

@markdoerr I almost finish but I need to stop for today. Already at more than 500 code of tests
I think it only miss me 2 units test and 2 particular cases in the already existing case.
Hope to finish that on wednesday but as I leave on holliday the 20th of january we will maybe merge but not release the new version.

@AMontagu
Copy link
Collaborator Author

@markdoerr I did not manage to find the time to finish this today. Following my agenda I think I will not have the time to finish before holliday.

@markdoerr
Copy link
Contributor

Dear @AMontagu,
thanks anyway for all your efforts !
I know, that you are working hard and deserve your holidays. I will make then a kind of "intermediate" release and ask my colleagues to wait until you are back.

@AMontagu
Copy link
Collaborator Author

@legau You can review again.
I make all my changes in a separate MR that I merged here to let you check only the changes even if a complete read again should be a better idea. (#258)

If you have only small changes can you do it by yourself and merge to let @markdoerr use master fot its release ?

AMontagu and others added 19 commits March 11, 2024 18:26
@AMontagu
fix default value
564dc8c
@AMontagu
working code for default value for required but need all implementation
39aa2b9
@AMontagu
wip workig partial update properly
7168be4
@AMontagu
working with partial update name variabilised
d520606
@AMontagu
working on tests
e92ba44
@AMontagu
all tests passing miss comments
fa9a3c6
@AMontagu
adding tests and fixing field updated even if not specified and add c…
7e3898f 
…omment
@AMontagu
adapt doc and adapt test
2c4e823
@AMontagu @legau
Update django_socio_grpc/mixins.py
208131d 
Co-authored-by: Léni <[email protected]>
@AMontagu @legau
Update docs/features/proto-serializers.rst
37b87dc 
Co-authored-by: Léni <[email protected]>
@AMontagu
not working
0218e1a
@AMontagu
WIP
1e7054e
@AMontagu
refacto tu use separate model
04fdeda
@AMontagu
WIP test retrieve
db76c33
@AMontagu
WIP miss some test and use case
3973917
@AMontagu
all tests on default value working
6a75d7a
@AMontagu
clean up
b907847
@AMontagu
rework doc
b9a6ce5
@AMontagu
allow default to be a callable and review improvement
2716fed
@AMontagu AMontagu merged commit 0d052ab into master Mar 11, 2024
4 checks passed
@AMontagu AMontagu deleted the fix/defaultValue branch March 11, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legau legau legau left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Model fields' default values not being used for optional fields in version 0.19.0
3 participants
@AMontagu @markdoerr @legau