Fork and refactoring

[apawsey]

Hi guys,

As mentioned over on the ArangoDB slack channel, we're busy building a EFCore database provider for Arango. We're making some good progress, but unfortunately I came across some issues with the architecture of this library that made it pretty difficult to do what we needed. Although I can see you are trying to open it up a bit with the various interfaces you added, I was still struggling to get to things I needed. As is unfortunately my usual behaviour, I started fiddling, and then I fiddled a lot more.

To be honest, I've fiddled so much, I'm not sure you'd be willing to take the changes as a pull request, not least because I deeply understand the personal feelings that come with someone messing with your code so much. I'm also well aware that my commit's are not at all organised!!! As I said, I didn't really start with the intention of making so many changes, so it all just ended up as one big lump essentially.

However, if you'd like you can have a look at my fork at : https://github.com/apawsey/arangodb-net-standard, and see what you think.

To give a quick recap of the changes:

• Standardised all responses on a base object, to make it easier to always understand success or failure
• Standardised all query/options objects on a base object, passing query sting parameters as name value pairs to avoid having to deal with querystring behaviour in those classes.
• Added serialization settings to the standard options so people can alter the behaviour as they want
• Opened up the ApiClient objects a little make it easier for people to inherit and add to them.
• Added cancellation support to all the async methods.
• Moved all standard request handing to the ApiClientBase object
• Added some options for customising the standard json serialization
• Added a tracing json serializer for debugging
• Changed the clients to return errors in the responses rather than throwing as exceptions by default, but the behaviour can be switched either way.

There is quite a few documentation issues to be cleaned up thanks to renaming/addition of parameters, and there is definately a few issues I haven't cleaned up. In fact there are a number of things I still want to do. But all the tests are passing at the moment.

Apologies for taking such as heavy handed approach, but let me know your feedback. As I said if you don't want to take on the changes, I'm absolutely fine with that, but I'll need to keep the fork going for now for the EF project.

Regards.

[DiscoPYF]

Hi @apawsey , thank you for taking the time to write this issue.

I had a look at the changes you created in your fork. It would be great if you can open a separate issue for each major changes so that we can discuss about them more specifically. I also think it would be more efficient if we want to get them into the main repo.

There are a few points I noted right away from the changes:

• You renamed the suffix of the models representing query parameters from "Query" to "Options".

A few issues back, we took the decision to use the "Query" suffix for all models carrying query parameters. We wanted to make the distinction between options used inside a post model from the parameters used in a query string. #28

• Standardised all query/options objects on a base object, passing query sting parameters as name value pairs to avoid having to deal with querystring behaviour in those classes.

There is an open issue that I think match what you did there: #64
I guess that in your case it's needed to be able to get all parameters in a generic way for the entity framework implementation?

• Added some options for customizing the standard json serialization

Are you referring to the ContentSerializationOptions property you added in the new RequestOptionsBase?
I'm not sure I understand the need for controlling the serialization options for query parameters. From what I saw, ArangoDB always expect a JSON serialized in a specific way.
We already provide the possibility to override the default serialization logic we use through the IApiClientSerialization interface. Maybe it's not enough for your need?

• Added cancellation support to all the async methods.

I think it's a good thing to have. Maybe you can open a separate issue so that we can get this in faster?

In overall, we tried to stay as close as possible to the ArangoDB HTTP API. This drived some of the design decisions we made.
That's why I think opening new issues would help us better understand the need for the changes you made, through more focused discussions.

It's interesting to see new needs appearing, as the library gets used in different contexts.

[apawsey]

Hi @DiscoPYF,
More importantly thank you for obviously taking the time to have a good look at my fork.

Yeah unfortunately as I started making changes (as I was trying to get the EF core provider working in parallel), it got harder to keep them all discrete, and open issues (& pull requests) for each one, so again, my apologies that it all ended up as one big lump.

I'll be honest as well that not all of the changes are "necessary" in the sense of the EF core work I was doing. A number of them just came from a personal viewpoint of organising the code - for example the api client methods having the same code repeated every few lines - repeated code is unfortunately a "trigger" :) for me. I worked on a project recently with thousands of lines of repeated code, and so as soon as I see that kind of thing, I start looking for another way.

I can open issues for each of the changes if you think that would be best. My apologies in advance though as I'm going to spam your issues board a bit then.

On that basis, I won't respond here to the points you raised, but rather I'll respond on the issues you already highlighted, and I'll create new ones for the other changes.

Again, thanks for taking the time to engage on this.

[apawsey]

I've posted some issues, and I'll check through and see if I need to add any more to cover other changes.

Thanks for your time and consideration on these points.

[DiscoPYF]

Hi @apawsey , that's great, we will have a look at those. 👍 Thank you.