Test_Toolkit: Allowing for some flexibility i naming of create methods

[kThorsager]

Compliance is currently dictating that the file name to a Create method is to be a exact match to the return type. This does not allow for much flexibility for methods returning more general types (i.e. like SteelSection, but for more logic heavy methods) which one might want to partition into more than one file. Could we make it be enough that the file name includes the return type?

[awakeman]

It used to be this way, the filename and Method Name matching, but instead, due precisely to the fact that Structures_Engine has multiple methods to create SteelSections in a SteelSection.cs file with multiple different method names in the one file. @IsakNaslundBh argued the rationale for this was to prevent having many different files each containing one method to create the same type of object. This was captured in #87 and fixed in #88. In short, the behaviour used to be the same as this issue requests, but the opposite argument was presented to change it to how it is now. Ultimately we can't really support both without it just being so permissive that it is no different to not being checked at all. The solution depends, instead, on which "rule" is more in-line with BHoM philosophy. Interested to get @adecler's view on this too.

[FraserGreenroyd]

Just had a call with @awakeman , @IsakNaslundBh , @kThorsager and myself to discuss this. We have come up with and agreed with the following compliance in these cases.

EITHER

• Create method file names must match their return type (e.g. Blah.cs contains only Create methods which return Blah objects)

OR

• The Create method filepath must contain the return type AND the method name must match the file name

To be PRed up by @awakeman or myself depending on who gets there first.

[adecler]

As far as I am concerned, the rule has always be the same since pretty much day 1:

• file name must match the return type
• method name must at least partially correspond to the return type (following PascalCase splitting)

See for example one of our oldest file: Transformation matrix. As mentioned previously, the rule was encoded that way in the early day version of the compliance test toolkit.

[awakeman]

Indeed that was the rule but considering the spirit of the rule rather than the letter of it is necessary too. As I see it the rule, as with all of the rules, exists for 3 purposes:

1. To ensure a consistency across the codebase
2. To enhance readability of the code
3. To enhance discoverability of the code

Where 1 is, at least in part, in service of the other two. Consistent code, and organisation thereof, is more readable (or at least comprehensible) and more discoverable/navigable.

The reason for having the file names correspond to either the return type of a method (what it creates) or the method name is primarily in service of discoverability, i.e. being able to find the code that corresponds to a particular node on, say, the grasshopper canvas or being able to find all code that creates a particular type.

Secondarily organising the code this way generally results in smaller, single purpose source files, enhancing readability on account of avoiding the cognitive load of a very long files.

The case that triggered this discussion and change was that there are some files that were flagged as incompliant because are broken out into separate short files, each of which create the same type of object in a different configuration, using a relatively long and complex methods to achieve it. To comply with the simple rule as you have stated it would result in one very long file, doing a disservice to the goals of readability and discoverability for the sake of a rigid sense of consistency.

The rule we have arrived at adds flexibility within some boundaries which can be consistently applied whilst maintaining smaller, single purpose, source files. We have made sure that if this exception needs to be invoked in service of purpose number 2 that there are then two conditions that must be met in service of purpose 3: Namely that the file path must organise the files inside a directory of the name of the return type, maintaining the relationship to what type of object the method creates; and the source file must then be named the same as the method, introducing for this class of method the 1:1 relationship between the file and the name of the component in the UI.

[epignatelli]

After a chat with @FraserGreenroyd, reopening this because I believe I have another edge case.

In the Rhinoceros_Toolkit we create rhino objects. The method that creates a random geometry type returns object, whereas the file is called Geometry.cs.

I believe it makes sense to keep it called Geometry.cs as it is more informative about the content than object.cs or object/Geometry.cs

https://github.com/BHoM/Rhinoceros_Toolkit/blob/485a9fe7b21fa9775a821c5633afb3f5d4703fc8/Rhinoceros_Engine/Create/Geometry.cs#L33-L36

https://github.com/BHoM/Rhinoceros_Toolkit/blob/master/Rhinoceros_Engine/Create/Geometry.cs

Any though?

[epignatelli]

I found another potential exception.

If you look at the Rhinoceros_Toolkit, and specifically two Point3d.cs file, it contains two main methods, one returning a List<Point3d> and another returning a Point3d.

However they cannot have the same name, since they would only differ in the return type, which is not allowed in C#.

[FraserGreenroyd]

I found another potential exception.

I don't think you did - see my comment on that PR, I think that one is ok (with a potential misunderstanding)? :smile:

The other one though I do agree, we need another chat on @awakeman @IsakNaslundBh @adecler et. al.

[epignatelli]

Yep

I found another potential exception.

I don't think you did - see my comment on that PR, I think that one is ok (with a potential misunderstanding)? 😄

Yep, just seen that, thanks! That worked!

[FraserGreenroyd]

After a chat with @FraserGreenroyd, reopening this because I believe I have another edge case.

In the Rhinoceros_Toolkit we create rhino objects. The method that creates a random geometry type returns object, whereas the file is called Geometry.cs.

I believe it makes sense to keep it called Geometry.cs as it is more informative about the content than object.cs or object/Geometry.cs

https://github.com/BHoM/Rhinoceros_Toolkit/blob/485a9fe7b21fa9775a821c5633afb3f5d4703fc8/Rhinoceros_Engine/Create/Geometry.cs#L33-L36

https://github.com/BHoM/Rhinoceros_Toolkit/blob/master/Rhinoceros_Engine/Create/Geometry.cs

Any though?

After giving this further review and discussion with @epignatelli I feel it comes down to a clear definition between Create and Compute. The methods outlined in the file @epignatelli linked to are, in my view better suited to the Compute class, because they are calculating which type of random geometry they return. Now, there is a bit of blurred lines here, because ultimately they are returning created geometry, and the methods rely heavily on the Create methods to produce their data. But, the actual functionality contained is taking a random number and a switch statement to return something randomly. This to me, is calculation, not creation.

@epignatelli has agreed so that file will move in Rhino toolkit and allows us to continue with the convention we've set up above which handles our primary use cases, and doesn't suddenly give us a lot of edge cases to try and cater for.

I also like, in my head, this way of looking at it, as it helps with a fear @IsakNaslundBh has mentioned before of not just "moving everything which doesn't comply to a Compute method to pass compliance" (paraphrased) - providing the method is performing a calculation, it should be a compute.

Later, we'll argue compute vs query I'm sure... but that's a different issue!