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

Provide ability to override options used to serialize, use default op… #290

Merged
merged 15 commits into from
Nov 12, 2020
Merged

Provide ability to override options used to serialize, use default op… #290

merged 15 commits into from
Nov 12, 2020

Conversation

modios
Copy link
Contributor

@modios modios commented Oct 7, 2020

An alternative implementation so we can use custom properties for the serializer.
This has several differences compared to #285.
Please let me know which one you prefer so we can decline the other PR and try to merge the one chosen (if we are happy with any of them).

Copy link
Collaborator

@rossmills99 rossmills99 left a comment

Choose a reason for hiding this comment

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

I prefer this to the other set of changes; it provides a means to set default serializer settings as well as override them in the specific places where user-specified data is supplied.

There's one use case that might not be covered which is setting a user-specified object in an AQL query bind-params. For example:

var response = await _cursorApi.PostCursorAsync<MyModel>(
                "INSERT @obj IN Collection",
                new Dictionary<string, object> { ["obj"] = new MyClass() });

We added specific handling for that case because we wanted to avoid changing the bind vars during serialization, since that's obviously confusing when you come to use the keys in the query. @DiscoPYF might remember better the specifics of what that issue was about, did I get it right?

Anyway I've left a few comments about typos in the PR.

The last thing is that the readme should be updated to reflect changes in the Quick Start guide and to add some documentation about how to set default serializer settings in serializer implementations.

Copy link
Collaborator

@DiscoPYF DiscoPYF left a comment

Choose a reason for hiding this comment

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

I also prefer this approach to the other pull request. The functionality brought by those changes is great. Thanks @modios

I left a bunch of comments. I would appreciate if we can go over those.

Copy link
Collaborator

@DiscoPYF DiscoPYF left a comment

Choose a reason for hiding this comment

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

@modios Thanks for updating. I left a few more comments but they don't go in the way of merging this, so I have approved. 👍

@DiscoPYF
Copy link
Collaborator

DiscoPYF commented Oct 8, 2020

fix #276
fix #203

@rossmills99
Copy link
Collaborator

This is looking good! The only thing missing is an update in the readme to reflect the new options and changes. I was going to recommend some changes, but since I've gone to the effort of reading the doc again I'll just push a new commit with what I would otherwise be proposing.

@DiscoPYF DiscoPYF merged commit d0dc294 into ArangoDB-Community:master Nov 12, 2020
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.

4 participants