Remove the constructors from options

[dangershony]

No description provided.

[sondreb]

Looks great! =) This is a good change, less plumbing and easier future updates.

[dangershony]

All tests passed this can go in

[sondreb]

Have removed the setup of .NET Core, tooling is already installed so we save some time and processing pr. build.

Have upgraded the runtime from 3.1.0 to 3.1.4, as we have already upgraded the packages to this version.

[MithrilMan]

Is it possible to have a comment about the reason of the change or a link to an issue?
I think it's important to have an history about changes

[dangershony]

Yes good point the reason we made this change is that some properties where missed in constructors and lead to incorrect values being set on some chains, for example form discord

[MithrilMan]

I see, but how can this change help with that scenario?
It seems to me that it may happen the same if you forget to set a default value when you create a new option (it's the same as to forget to set a value in the constructor with previous implementation)

Probably a way to be "sure" that a new option has been inizialized is by having a method that uses reflection to get all the properties defined in the class type (and ancestor(s)) and check that their value is different by the default value of such property (e.g. for int default is 0, for string is null, etc...)
An improvement would be to dynamically generate code for such properties in a way that a flag (for each property) is set when these properties are set and so you can have a view over the instance to know if a specific value has been set.

I'm not suggesting these approach tho, need to balance the complexity against pros and cons, probably a generic test that check by reflection?

In any case I don't see how this change help to not incurr in the same problem in future, the only pros actually is that it has less code to maintain but still won't fix the problem

[sondreb]

I see, but how can this change help with that scenario?
It seems to me that it may happen the same if you forget to set a default value when you create a new option (it's the same as to forget to set a value in the constructor with previous implementation)

No, the problem is that default option was added in the constructor, but not overload in the other constructors. There was 3 constructors on the object, one that set defaults, another which set some, not all, of the values. So every time a new option is introduced, it requires edits in a huge amount of classes, including every single blockchain that is built on Blockcore must change their initialization code.

Edit: Just to clarify, Dennis didn't forget to set the default, it's just that there are multiple constructors.

There is no need for reflection or complex change, we simply rely on setting default properties on the options class in the property declarations, and then chains built on Blockcore must make the conscious decision what of the defaults they want to change. As long as we don't change the defaults, we should be fine.

[MithrilMan]

ok so this was the same as having a default constructor where all variables are set with the default values and remove the other constructors.

Honestly I find more readable, when you have quite a lot of properties, to have a constructor where all values are set rather than inlining them on the property definition, even more where you have scenarios like above where one property depends on another.

Anyway if you all agree on current implementation ok

[dangershony]

I am becoming more and more in favor of property direct initialization rather then in constructor.
I think also the language is going down that pass (see next c# release notes).

[MithrilMan]

I'm not against it, I use it a lot in my projects, my point is more about code consistency so if we decide to initialize inline (and note that isn't always possible) then we should probably adopt it anywhere in the code.

[MithrilMan]

Anyway what we have to avoid completly is having inline declaration and then some constructor(s) override that inlined values, because it will cause just a mess.

[dangershony]

then we should probably adopt it anywhere in the code.

yeah agree, I'd even say we should try to refactor the way we init the consensus class its a bit wired right now (massive constructor, to much abstraction with the IConsensus interface and abstract class)