htm.core icon indicating copy to clipboard operation
htm.core copied to clipboard

Making validateParameters( ) implicit.

Open dkeeney opened this issue 5 years ago • 1 comments

Question: NetworkAPI, calling arguments passed into a Region implementation. Couldn't the parameter validation be done implicitly?

Parameter validation does several things:

  • Throws an error if an unknown field name is passed in. All names must be listed in the spec.
  • If a known field is not passed in but has a default value in the spec, insert this value into the set of parameters.
  • Confirms that if a field is a scalar (i.e. count property is 1) the Default value is a scalar. If not a scalar the Default should be an array (or map).
  • Throws an error if the passed in field is ReadOnly.

Background: Originally, the parsing of the creation parameters was performed in RegionImplFactory just prior to creating the Region. The spec was integrated into the parsing and had major shortcomings. Binary conversions happened at this point. My feeling was that the values passed in may need some context from the class in order to properly interpret the defaults and it was difficult to predict which data type was needed for each field. So I re-wrote the parsing routines to separate the yaml parsing from the validation. I then passed just the raw values (as parsed strings) to the constructor without defaults and required the constructor to call validateParameters( ) to perform the functions described above.

Now I am not sure that was the right choice. Separating the parsing and spec validation was good because it made the parsing more general, but requiring the Region Impl constructor to be responsible for validating and applying defaults only complicated things. As it turned out, there was never a need for the Region to know if a field had been set to its default and it was always defaulted to the value in the spec. A general EncoderRegon would have needed more control but that is unlikely to happen.

So I am considering moving this back into the factory. The Region impl constructor would always be presented with a validated set of parameters with defaults applied. The effect of this change would be to go back and modify all C++ Region Implementations and PyBindRegion.cpp (which is a stand-in for python implemented regions). The actual type conversions would still remain in the Region Impl constructor. Value ranges and acceptable value checks would still be performed by the constructor.

Is it worth the effort?

dkeeney avatar Jun 19 '20 17:06 dkeeney

@dkeeney personally, I do not have any problem if we call validateParameters() in Region constructor. What is better if you come back into the factory? If it provides no better Performance, then it may be a nice feature, and no need to make it beautiful. We have many open points for solving! What do you think?

Thanh-Binh avatar Jun 19 '20 18:06 Thanh-Binh