Reorganize analyzer properties
- Got rid of the generic AnalyzerProperties class
- Implemented a custom properties class for each specific analyzer type
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
Analyzerobjects returned by API methods (e.g.PostAnalyzerAsync,GetAnalyzerAsyncorGetAllAnalyzersAsync). Only an emptyAnalyzerPropertiesBaseobject 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.Typeproperty, e.g. using a converter. However any serializer specific logic should be carefully reviewed because the library allows overriding the serializer, so someone may useSystem.Text.Jsonor 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.
@DiscoPYF, I have revived this PR for your review (our discussions). Here's the update:
- I've updated the PR from the
masterbranch. - I've brought
AnalyzerPropertiesback in as it currently stands in themasterbranch. - I've defined a new class
AnalyzerDefinitionthat will be used to POST analyzers with itsPropertiesproperty set to one of the custom analyzer properties objects. - The
Analyzerclass now inherits fromAnalyzerDefinitionwith itsPropertiesproperty set to aAnalyzerPropertiesobject hiding the same property in the base class. Please share your thoughts and suggestions. Thanks.