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

Prototyping reflection of external libraries #120

Merged
merged 29 commits into from
Aug 28, 2019

Conversation

epignatelli
Copy link
Member

@epignatelli epignatelli commented Aug 8, 2019

NOTE: Depends on BHoM/BHoM_Engine#1132

To test the prototype, switch to the correspondent (identically named) branches in the Grasshopper_Toolkit and the BHoM_Engine

Issues addressed by this PR

This mainly supports BHoM/BHoM_Engine#1132
Closes #118 as a side effect

This pr add the ExternalCaller component that contains all the methods from external libraries.
As discussed with @adecler I tried to make it as BHoMmy as possible by modifying a little bit the MethodCaller. This way the new component fits into the pattern of all the other Engine methods.

On the other hand, this pr migrates a set of methods to the Reflection_Engine, since they are much more general.

As usual, I left some comments on the diff itself to clarify the modifications.

Test files

The test comes with the PR on the Numpy_Toolkit: BHoM/Numpy_Toolkit#2

Changelog

  • Add the ExternalCaller component that can reflect any external assembly
  • Migrate the CompileFunction method to the Reflection_Engine as Engine.Reflection.Convert.ToFunc
  • Migrate the GetConstructedType method to the Reflection_Engine as Engine.Reflection.Modify.MakeGeneric
  • Migrate the Engine.UI.Create.MethodInfo to the Reflection_Engine as Engine.Reflection.Create.MethodInfo
  • Migrate the Engine.UI.Modify.GroupMethodsByName to the Data_Engine as Engine.Data.Modify.GroupByName and clarified it is a generic method

Additional comments

@epignatelli epignatelli added the type:feature New capability or enhancement label Aug 8, 2019
@epignatelli epignatelli added this to the BHoM 2.4 β MVP milestone Aug 8, 2019
@epignatelli epignatelli self-assigned this Aug 8, 2019
@epignatelli epignatelli changed the title BHoM_Engine: Prototyping reflection of external libraries [WIP] BHoM_Engine: Prototyping reflection of external libraries Aug 8, 2019
@al-fisher al-fisher added the status:WIP PR in progress and still in draft, not ready for formal review label Aug 12, 2019
@@ -238,11 +173,28 @@ public virtual void SetInputParams()
HasDefaultValue = x.HasDefaultValue,
DefaultValue = x.DefaultValue
}).ToList();
if (Method is MethodInfo && !Method.IsStatic)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inserting the instance as an input if the method is an instance method

}
}

/*************************************/

public virtual void SetDelegate()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old CompileFunction now calls the ToFunc from the Reflection_Engine

@@ -1,92 +0,0 @@
/*
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migrated to the Reflection_Engine

@@ -1,85 +0,0 @@
/*
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migrated to the Data_Engine

@@ -37,7 +37,7 @@ public static partial class Query

public static IEnumerable<MethodBase> EngineItems()
{
return Engine.Reflection.Query.BHoMMethodList().Where(x => !x.IsNotImplemented() && !x.IsDeprecated());
return Engine.Reflection.Query.BHoMMethodList().Where(x => x.IsExposed());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the new centralised way to filter out methods we don't want to expose

@epignatelli epignatelli marked this pull request as ready for review August 16, 2019 14:28
@epignatelli epignatelli removed the status:WIP PR in progress and still in draft, not ready for formal review label Aug 16, 2019
@epignatelli epignatelli removed this from the BHoM 2.4 β MVP milestone Aug 19, 2019
@epignatelli epignatelli changed the title [WIP] BHoM_Engine: Prototyping reflection of external libraries BHoM_Engine: Prototyping reflection of external libraries Aug 19, 2019
@adecler adecler changed the title BHoM_Engine: Prototyping reflection of external libraries BHoM_UI: Prototyping reflection of external libraries Aug 27, 2019
Copy link
Member

@adecler adecler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, just a couple of minor things.
Would be good to catch up before the merge though.

{
m_CompiledFunc = Method.ToFunc();
}

public virtual void SetOutputParams()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to put the separator comment here.
Although to be honest, I am not sure I see the need of this method as it is a one liner is is only called once

/***************************************************/
/**** Public Methods ****/
/***************************************************/
public override string Name { get; protected set; } = "ExternalCompute";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer it to be name External instead of ExternalCompute.
I understand where you're going with this but not every linked method is going to correspond to a Compute format so I'd rather stick to External to avoid the parallel.

@epignatelli
Copy link
Member Author

Thanks @adecler for the review!
Comments addressed in the two last commits

Copy link
Member

@adecler adecler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @epignatelli , all good now!

@adecler adecler merged commit 48c778c into master Aug 28, 2019
@adecler adecler deleted the BHoM_Engine-#1131-ReflectingExternalLibraries branch August 28, 2019 02:04
@FraserGreenroyd FraserGreenroyd changed the title BHoM_UI: Prototyping reflection of external libraries Prototyping reflection of external libraries Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BHoM_UI: The UI does not seem to like methods returning void
3 participants