Refactor formatter config to reqnroll json config#1024
Refactor formatter config to reqnroll json config#1024clrudolphi wants to merge 6 commits intomainfrom
Conversation
…g for deserializing the reqnroll.config content to obtain the Formatters configuration.
…ion from an untyped Dictionary<> to a typed contract that supports untyped additional properties.
|
This accomplishes what I had set out to do. Would you like to see it fully incorporated? |
No, that's OK. I will check it next week. |
Re-ping on this. Is there more work you'd like to see on this before we move forward? |
gasparnagy
left a comment
gasparnagy
left a comment
There was a problem hiding this comment.
I think the direction is good. I have made some comments.
|
|
||
| try | ||
| { | ||
| var jsonConfig = JsonSerializer.Deserialize(jsonContent, JsonConfigurationSourceGenerator.Default.JsonConfig); |
There was a problem hiding this comment.
Do I get it right, that this is going to be removed once the PR is complete, as we can simply get the deserialized config from Reqnroll, right?
There was a problem hiding this comment.
That was not my intent; but I do see where this is awkward. I was attempting to use this extractor for two situations: regular configuration via the reqnroll.config file and for the situation of when a user overrides formatter configuration using an environment variable REQNROLL_FORMATTERS (as a json string).
This PR only goes so far as to re-use the Json source generators for deserialization. The formatters configuration subsystem is independently finding and reading the reqnroll.config file content.
I don't yet have the ReqnrollConfiguration object modified to have a Formatters property.
Even/when we go that far, we still need this "extractor" code to handle the override of Formatters config via Environment variable.
There was a problem hiding this comment.
Yes, we would anyway need this code, but only use it for the env overrides.
But I'm fine going step-by-step and keep it like as it is for this PR.
There was a problem hiding this comment.
I went ahead and integrated the formatters configuration reading with ReqnrollConfiguration and the ConfigurationLoader.
The extractor is retained for use by the Json Environment variable override provider.
| /// Captures any additional/custom formatters not explicitly defined above. | ||
| /// </summary> | ||
| [JsonExtensionData] | ||
| public Dictionary<string, JsonElement> AdditionalFormatters { get; set; } |
There was a problem hiding this comment.
Why is this not a Dictionary<string,FormatterOptionsElement>?
There was a problem hiding this comment.
I haven't found a way to do that directly. The Json source generator limits extension data to JsonElements, JsonNodes, or objects.
There was a problem hiding this comment.
But we could make the whole thing as a Dictionary<string,FormatterOptionsElement>, right? That would implicitly contain html and message as keys, but we could make prop getters to access them easily.
2 participants
🤔 What's changed?
Refactored the primary interface of Formatters configuration from an untyped dictionary to a class with support for additional properties.
The 'formatters' element of the reqnroll.json has been added to the JsonConfig structure so that Reqnroll's configuration deserialization logic is used to load the formatter config.
These changes cause a breaking change in the contract of FormatterBase, so this should only be targeted at next major release.
⚡️ What's your motivation?
Better alignment with overall Reqnroll configuration mechanisms.
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
At this point, the source generator JsonConfig is used for deserialization, but the formatters element is not yet added to the ReqnrollConfiguration class structure.
📋 Checklist:
This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.