CIHDigitalConfigurator / CIH_Toolkit

CIH_Toolkit
GNU Lesser General Public License v3.0
0 stars 0 forks source link

Creation of initial Specification and Conditions prototype #2

Closed alelom closed 3 years ago

alelom commented 3 years ago

NOTE: Depends on

https://github.com/BHoM/BHoM/pull/1165

Description

Initial implementation of the Specification and Conditions.

Test file

https://mottmac.sharepoint.com/:f:/r/sites/PlatformDesignProgramme/Shared%20Documents/0.10%20Digital%20Configurator/WP5%20Reference%20Implementation/Scripts/%231-InitialImplementation?csf=1&web=1&e=hgCMsd

image

image

al-fisher commented 3 years ago

@alelom This is looking good - will have a bit more of a play with it tomorrow. 😸

Initial comment around the handling of the Boolean operators -

Structured so as to accept a List<IFilter> and an enum in this way does leave the door open to some ambiguity for some of the operators. For instance an IMPLIES must only have two arguments and is not a commutative operator, and so both the number and order of the inputs matters. You can create the below which is not clear

image

might therefore be easier to design as discrete objects and properties? What do you think?

al-fisher commented 3 years ago

Thinking further on the formatting of above could be good to also think about structure of concepts for reflection into Excel_Toolkit!

Building specs in tabular format through Excel templates will be very powerful. Noting here - but perhaps one for later iterations

alelom commented 3 years ago

For instance an IMPLIES must only have two arguments and is not a commutative operator, and so both the number and order of the inputs matters. You can create the below which is not clear

I see what you mean, but I think it is not different from any other operator, as they are all defined for 2 inputs only. The IMPLIES may simply work so if more than 2 inputs are defined e.g. A, B, C, D they resolve as [(A => B) => C] => D. Only difference as you say is that the other operators are commutative, and for this the list order is important when using the IMPLIES – a fact that an user that wants to use it should be able to understand, and the user has still full control to use it correctly. Plus, it's less common as an operator, and I included it only for completeness. Happy to chat.

alelom commented 3 years ago

While implementing checks: the concept Domain as in our Domain class may be too restrictive. Common scenario can be a Domain check for e.g. DateTime. I'm implementing it with System.Object. Only requirement for the check to work is that the type implements the >, <, == operators (as DateTime does). This way it is extendable to any measurable quantity. Alternatively we can impose a cast to double. Happy to discuss.

al-fisher commented 3 years ago

@alelom Added a couple of comments 👆 👆 in the absence of BHoMBot 🤖 😄

alelom commented 3 years ago

The PropertyName the Condition applies to should I think be defined "Flat" in parallel with the actual Value, Tolerance, Domain etc. Think this is a more logical structure for the user - see original prototype in issue here: #1 We don't want the two separating.

That was actually my first prototype. I then moved away from that because, in order to cover all "comparisons" (Value, Domain and Set) you end up having:

For a total of 9 classes.

Therefore, I refactored, thinking that this current approach:

However, I did realise that, as you say, there is repetition of Source and Comment at the two levels. To solve that, I had in mind to perhaps qualify ValueCondition, DomainCondition and SetCondition as "Comparisons" instead (ValueComparison, DomainComparison, SetComparison) – objects that acquire value only when attached to a parent condition.

If we still want to push the solution you propose, there is also code duplication to consider; to avoid it, things in the back-end will actually not change much from the current structure - 3 single methods for Value, Domain, and Set comparisons, receiving calls dispatched from the 9 classes.

al-fisher commented 3 years ago

Hey @alelom - just capturing our conversation earlier.

Suggested moving back to list of simple Conditions ValueCondition DomainCondition SetCondition etc.

All the above then have as a defining Property: string PropertyName that the Condition applies to.

This PropertyName can refer to an object's Defining Property, Derived Property, Custom Data, or Property of a Fragment it does not matter. This is all handled in your GetValue method.

This negates the need for PropertyCondition, CustomDataCondition, FragmentCondition layer

Also means we can neatly implement exposure of Reflection of Engine methods for applying Conditions to Properties like Length() etc. very intuitively for users.

al-fisher commented 3 years ago

Think above captures it?

Can also catch up and finish our chat on Specifications owning Specifications as needed ... 😄

alelom commented 3 years ago

@al-fisher thanks for spotting those, all fixed now.

al-fisher commented 3 years ago

Brilliant - thanks