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

Reflection_Engine: Added TryRunExtensionMethod #2357

Merged

Conversation

alelom
Copy link
Member

@alelom alelom commented Feb 26, 2021

Issues addressed by this PR

Closes #2348

Adds a TryRunExtensionMethod. If no method is found, returns false. Otherwise, invokes the method and passes the output in the out parameter, then returns true.

Test files

Test with any choice of extension methods.

Changelog

Additional comments

@alelom alelom requested a review from pawelbaran February 26, 2021 15:58
@alelom alelom self-assigned this Feb 26, 2021
@alelom alelom added the type:feature New capability or enhancement label Feb 26, 2021
Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

I have a found minor misconceptions in the descriptions - please see the code comments (plus possibly consider copy-pasting stuff from RunExtensionMethod).

On top of that, I would consider two changes in the code itself:

  • Adding a similar method taking allowing to call an extension method with other parameters than the target (similar as in RunExtensionMethod).
  • Refactoring the method to return Output<bool, object> instead of bool and out param - this would allow using the method from the UI level, which may be a nice to have. However, in this case I would lean on @adecler to have the final say.

Reflection_Engine/Compute/TryRunExtensionMethod.cs Outdated Show resolved Hide resolved
Reflection_Engine/Compute/TryRunExtensionMethod.cs Outdated Show resolved Hide resolved
@alelom alelom requested a review from pawelbaran March 5, 2021 16:07
@alelom
Copy link
Member Author

alelom commented Mar 5, 2021

Refactoring the method to return Output<bool, object> instead of bool and out param - this would allow using the method from the UI level, which may be a nice to have. However, in this case I would lean on @adecler to have the final say.

My preference is to keep the out parameter instead, to follow C#'s convention. In particular, I expect this method to be used 95% of the time from the code, rather than from the UI. Also, I believe that the ideal solution for scenarios like these would be for the framework to support the correct reflection of out parameters in the outputs of a component (as far as I understand, currently out parameters end up among the inputs instead).

@pawelbaran
Copy link
Member

Agreed with @alelom, the standard C# convention with out parameter makes sense to me in this particular case, so the code LGTM now.

However, due to the fact that the PR introduces a UI glitch (nonharmful, the UI is not crashing, it just looks weird), I would like @adecler to have a final say before we approve/merge.

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.

I am fine with the out parameter. It better aligns with the general C# conventions on the Try methods and we will eventually support out parameters in the UI anyways. In the meantime, people can just use the RunExtensionMethod component in the UI if they ever need that for some reason.

@pawelbaran
Copy link
Member

@BHoMBot check all

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 8, 2021

@pawelbaran to confirm, check-code-compliance, check-documentation-compliance, check-project-compliance, check-branch-compliance, check-dataset-compliance, if applicable check-copyright-compliance, check-serialisation, check-versioning, check-unit-tests, check-core, check-installer, and check-ready-to-merge tasks are now queued.

@pawelbaran
Copy link
Member

/azp run BHoM_Engine.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

Confirming LGTM, happy to see this merged.

@FraserGreenroyd FraserGreenroyd merged commit ebedc78 into master Mar 8, 2021
@FraserGreenroyd FraserGreenroyd deleted the Reflection_Engine-#2348-TryRunExtensionMethod branch March 8, 2021 09:36
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.

Reflection_Engine: introduce TryRunExtensionMethod
4 participants