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

Reorganize analyzer properties #450

Open tjoubert opened 1 year ago

tjoubert commented 1 year ago
tjoubert commented 1 year ago

Good idea to refactor into separate classes for analyzer properties 👍

I left a few minor comments. One thing I want to discuss before merging:

With this new approach, analyzer properties will not be available from Analyzer objects returned by API methods (e.g. PostAnalyzerAsync, GetAnalyzerAsync or GetAllAnalyzersAsync). Only an empty AnalyzerPropertiesBase object will be available. The serializer has no way at the moment to know what derived type to create when deserializing.

Is it expected?

A workaround is to instruct the serializer to construct a derived type based on the Analyzer.Type property, e.g. using a converter. However any serializer specific logic should be carefully reviewed because the library allows overriding the serializer, so someone may use System.Text.Json or any other.

Perhaps we should think a bit more about this approach? For now, I propose to:

* Close this pull request

* Open a new pull request to fix the bug by simply adding the properties on the existing `AnalyzerProperties` object.

* Open a new issue to refactor the properties like you did, which will give some more time to think about. We can link this pull request on the issue.

What do you think?

Also, isn't it a breaking change as we are changing the way analyzer properties are provided? Probably all the more reasons to hold off until the next major release. I've added the "breaking change" label.

@DiscoPYF , you are right. In order to merge this PR correctly, we would need serializer-specific logic to deserialize the Analyzer.Properties based on what is defined in Analyzer.Type. This is also a breaking change (thanks for the tag). We will leave this PR open and put it on hold. I am going to open a new PR that will leave AnalyzerProperties.cs as it was and simply add all the new properties to it.

tjoubert commented 1 year ago

@DiscoPYF, I have revived this PR for your review (our discussions). Here's the update: