Base_oM: Adding Enumeration object #1294
Conversation
adecler
added
the
status:do-not-merge
adecler
removed
the
status:do-not-merge
adecler
force-pushed
the
EmbodiedCarbon_Toolkit-#1-InitialVersion
branch
from
dbdedb5 to
2d394c0
Compare
adecler
force-pushed
the
EmbodiedCarbon_Toolkit-#1-InitialVersion
branch
from
a1bbf01 to
17155b1
Compare
| public static bool Equals(IEnum a, IEnum b) | ||
| { | ||
| if (a == null || b == null) | ||
| return false; | ||
|
|
||
| return a.GetType().Equals(b.GetType()) | ||
| && a.Value.Equals(b.Value); | ||
| } | ||
|
|
||
| /***************************************************/ | ||
|
|
||
| public static IEnumerable<T> GetAll<T>() where T : Enumeration | ||
| { | ||
| return typeof(T).GetFields(BindingFlags.Public | BindingFlags.Static | BindingFlags.DeclaredOnly) | ||
| .Select(f => f.GetValue(null)) | ||
| .Cast<T>(); | ||
| } | ||
|
|
||
| /***************************************************/ |
There was a problem hiding this comment.
As discussed, these methods really should live in the Engine. Ok for that to happen in a subsequent PR, but if an issue can be raised to capture that!
There was a problem hiding this comment.
I am not too sure about that.
Enumeration.GetAll<T>() would translate into something like BH.oM.Base.Query.Values<T>() and therefore give a component like
One key point of separating the methods into the Engine is to provide a matching functionality in the UI but I'd rather not have that Values component in the UI as it makes absolutely no sense. We have an EnumerationValues method already (it is still in the EmbodiedCarbon toolkit though). So, ultimately, I'd rather delete GetAll<T>() then.
On a similar vein (although I am a bit more hesitant on this one), I prefer static bool Equals(IEnum a, IEnum b) to be in the object definition. The whole purpose of this method compared to the non-static alterative is that it allows to make it more readable in the code that we are comparing Enumerations by having a clear call to Enumeration.Equals and shorter way to handle null values. If we take that away by moving it to an Engine method Query.Equals, we might as well remove that method altogether. Admittedly, there isn't much reason right now to keep it so happy to do just that. It could become handy if we add implicit casting from string (something I would like to add later) though.
BHoM/Interface/IEnum.cs
Outdated
|
|
||
| namespace BH.oM.Base | ||
| { | ||
| public interface IEnum : IComparable |
There was a problem hiding this comment.
Question here, not really a change request, but any reason why this is not implementing IComparable<IEnum> instead of for object? Also, with the additional override methods required, could it not also implement IEquatable<IEnum> ? Seems like a close overlap.
There was a problem hiding this comment.
I have switched to IComparable<IEnum>.
I haven't added IEquatable<IEnum> because it requires an additional Equals(IEnum) and I don't see the point of that. It's not like we are using it anywhere else either.
|
@BHoMBot check required |
|
@adecler to confirm, the following checks are now queued:
|
There was a problem hiding this comment.
Have discussed with @adecler to raise an issue for virtual keyword compliance and merge this PR as is with dispensation on that check, with the virtual stuff being fixed in a smaller PR.
|
@FraserGreenroyd , I have raised teh issue about the missing virtual keyword here: #1324 |
|
FAO: @FraserGreenroyd The check they wish to have dispensation on is code-compliance. If you are providing dispensation on this occasion, please reply with:
|
|
@BHoMBot this is a CI/CD instruction. I am authorising dispensation to be granted on check ref. 4222680812 |
|
@FraserGreenroyd I have now provided a passing check on reference |
|
@FraserGreenroyd to confirm, the following checks are now queued:
|
|
@BHoMBot check ready-to-merge |
|
@FraserGreenroyd to confirm, the following checks are now queued:
|
Issues addressed by this PR
Closes #1293
See issue. Current implementation in the context of the Embodied Carbon toolkit
Test files
https://burohappold.sharepoint.com/:f:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/BHoM/Base_oM/%231294%20-%20Enumeration?csf=1&web=1&e=eX3XoG
You will need the following PRs to do the testing:
Additional comments
There's a lot that can be done to make the Enumerations more user friendly in the UI (e.g. make it behave exactly like a regular enum in the UI) but this PR is just about laying the foundation for this new type of object. It has been working great for me in the context of the Carbon calculator tool for all stages of workflow so I am pretty confident it is safe to merge. Make sure to play with the test file though.