Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global: Try to remove the dependency between the BHoM engine and the Reflection Engine #1645

Closed
adecler opened this issue Apr 9, 2020 · 8 comments · Fixed by #2729
Closed
Assignees
Labels
type:compliance Non-conforming to code guidelines

Comments

@adecler
Copy link
Member

adecler commented Apr 9, 2020

Migrate the method in the reflections that are used BHoM-Engine to the BHoM engine itself.
Also migrate most of reflection oM to the base.

@adecler adecler added the type:compliance Non-conforming to code guidelines label Apr 9, 2020
@adecler adecler self-assigned this Apr 9, 2020
@adecler adecler changed the title Try to remove the dependency between the BHoM engine and the Reflection Engine BHoM_Engine: Try to remove the dependency between the BHoM engine and the Reflection Engine Apr 9, 2020
@adecler
Copy link
Member Author

adecler commented Jun 20, 2020

The Reflection Engine methods called from the BHoM Engine are:

  • RecordError & RecordWarning
  • Attributes
  • RunExtensionMethod

RandomObject also uses BHoMTypeList() but this is a duplicate of DummyObject from the Test_Toolkit. Personally, I think Test toolkit is better suited to host that kind of object. It is also worth mentioning that RandomObject is never called in the code.

@adecler
Copy link
Member Author

adecler commented Jun 20, 2020

While the migration happens, it might be worth having another look at the RunExtensionMethod. See this issue: #692

@adecler
Copy link
Member Author

adecler commented Oct 6, 2020

@al-fisher to action.

@adecler
Copy link
Member Author

adecler commented Nov 13, 2020

@al-fisher, I think it would be good to start putting in place a strategy for actioning this.

Ideally, we would like to do this in three stages:

  • Add a duplicate of the three groups mentioned above in BHoM Base without deleting the originals from the reflection namespace
  • Give a period of time for everyone to migrate
  • Delete the original from reflection

This strategy could be a problem when both namespaces are referenced with using though as the code of other toolkit might not compile anymore. That being said, I don't think it will be teh case:

  • RecordError seems to always be referenced with the full Engine.Reflection.Compute namespace attached to it. Although it is always possible I missed some.
  • The namespace BH.oM.Base.Attributes doesn't exist yet so no risk there.
  • RunExtensionMethod also seems to always be referenced with the full Engine.Reflection.Compute namespace attached to it.

Ultimately, we can always do a PR on oM and Engine and run the installer check to see if it is causing problems or not. If it works or only requires a few corrections, then we are good to go.

@adecler adecler changed the title BHoM_Engine: Try to remove the dependency between the BHoM engine and the Reflection Engine Global: Try to remove the dependency between the BHoM engine and the Reflection Engine Nov 24, 2020
@FraserGreenroyd FraserGreenroyd self-assigned this Dec 16, 2021
@adecler
Copy link
Member Author

adecler commented Dec 17, 2021

I would add a few methods to the list for consideration. First are the methods that are strongly correlated to those we are moving already:

  • Event related
    • ClearCurrentEvents
    • RemoveEvent
    • DebugLog
    • AllEvents
    • CurretnEvents
  • Extention methods related
    • TryRunExtentionMethod
    • ExtentionMethods
  • Attribute related
    • all query methods related to those attributes

Given that @FraserGreenroyd is mentioning moving BHoMTypeList in this post, it would make sense to also move those methods:

  • BHoMFolder
  • BHoMVersion
  • BHoMMethodList
  • AssemblyPath
  • AssemblyList
  • CurrentAssemblyFolder
  • IsAdapterAssembly
  • IsOmAssembly
  • IsEngineAssembly
  • IsUIAssembly
  • LoadAllAssemblies
  • LoadAssembly

Finally, there are a couple methods on BHoM object core functionality that I often wished were in the core dll instead of reflection. Is it worth doing them at the same time as the rest ?

  • ToText
  • SetPropertyValue

@adecler
Copy link
Member Author

adecler commented Dec 17, 2021

@FraserGreenroyd , to answer you question of additional repos to include, here's my list:

  • SQL_Toolkit
  • BuildingPerformance_Toolkit
  • EmbodiedCarbon_Toolkit

Thanks 😄

@FraserGreenroyd
Copy link
Contributor

Finally, there are a couple methods on BHoM object core functionality that I often wished were in the core dll instead of reflection. Is it worth doing them at the same time as the rest ?

  • ToText
  • SetPropertyValue

I would agree it's worth including these two as well, so will add them to the list 😄

@al-fisher
Copy link
Member

Thanks both - making sense.

Last loose end to tie up before actioning will be "what is left in the current Reflection namespace?" see also comment here BHoM/admin#17 (comment)

Suggest an action of creating a clear diagram showing new structure and purpose of code in Core and Reflection oM/Engines.
We can then review first thing in Sprint 1 of 5.0 to make sure all happy and aligns with hierarchy and conventions.

FYI individuals involved in discussions to date @FraserGreenroyd @adecler @al-fisher @alelom @IsakNaslundBh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:compliance Non-conforming to code guidelines
Projects
Framework Priority
  
Completed
3 participants