ArangoDB-Community / arangodb-net-standard

A consistent, comprehensive, minimal interface to enable .NET applications to use the complete range of features exposed by the ArangoDB REST API.
Apache License 2.0
76 stars 30 forks source link

Fix typo in PostDocumentQuery query params builder. #491

Closed rossmills99 closed 6 months ago

rossmills99 commented 6 months ago

I noticed the OverwriteMode option not working for POST document operation and on investigation discovered a typo in the query-string builder.

DiscoPYF commented 6 months ago

Wow good catch!

A method that takes two parameters would've helped avoid this.

Example:

query.AddQueryParam("overwriteMode", OverwriteMode.ToLower());
public static class StringBuilderExtensions
{
    public static StringBuilder AddQueryParam(this StringBuilder builder, string name, string value)
    {
        builder.Add($"{name}={value}");
    }
}

I think it's worth opening a small separate improvement. What do you think?

rossmills99 commented 6 months ago

Wow good catch!

A method that takes two parameters would've helped avoid this.

Example:

query.AddQueryParam("overwriteMode", OverwriteMode.ToLower());
public static class StringBuilderExtensions
{
    public static StringBuilder AddQueryParam(this StringBuilder builder, string name, string value)
    {
        builder.Add($"{name}={value}");
    }
}

I think it's worth opening a small separate improvement. What do you think?

Yeah that probably would have been a good idea, although we may not benefit from it now that most of these are already implemented the way they are. Also we use a List<string> and then join with &s, but I think your approach of an extension method would work for List<string> instead of StringBuilder, or maybe better, implement a QueryStringBuilder class. But might not be worth the effort at this stage of the project where most of it is implemented already.