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

Add protected function instance constructor #759

Merged
merged 1 commit into from Jul 19, 2020

Conversation

viceice
Copy link
Contributor

@viceice viceice commented Jul 16, 2020

  • add protected constructor to FunctionInstance
  • add custom uuid type as test

closes: #753

Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

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

Awesome! This is just the coverage we need more to make sure CLR interop story works for people extending Jint.


public UuidTests()
{
_engine = new Engine(o => o.AddObjectConverter(new UuidConverter()))
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about adding an extension method on Options named AddUuid() which would register the converter and also call CreateUuidConstructor ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Would need some API refactoring.

Copy link
Collaborator

@lahma lahma Jul 16, 2020

Choose a reason for hiding this comment

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

Can we get away with AddObjectConverter<T>? not sure if we can bind the constructor with that though.

Copy link
Owner

Choose a reason for hiding this comment

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

@viceice I think we would need a way to add Action to a list that would be executed by the engine when it has initialized the options object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about to add Engine.ObjectConverters as public List<IObjectConverter> ObjectConverters => Options._ObjectConverters? so we can add IObjectConverter after Engine contructor finished?

Then the AddUuid() extension would be very easy.

Copy link
Owner

Choose a reason for hiding this comment

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

I prefer not to since we are looking at ways to reuse engines. Having all the configuration kept in options would allow us to reuse it on any engine instance once cleared of any state. We could even add a list of scripts. Or use these options in a factory object for pooling.

Instead of calling these methods from Engine, we can also have a method Apply(engine) such that the engine code would not change every time we add options. And a Options.Configure (Action) to register custom logic. I can add this to your PR if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, feel free to change the PR. 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I need to give you write access to my fork, because it's in an org.

Copy link
Owner

Choose a reason for hiding this comment

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

If you haven't unallowed me, then by default I can write on the branch you used to submit a PR

@sebastienros
Copy link
Owner

Had not realized that all these UUid classes were only in Tests, until I start adding what I suggested.
Doing it in a different PR then.
You should write a wiki entry with this UUid example and how to get to it, would be very helpful for many.

@sebastienros sebastienros merged commit 5d1114d into sebastienros:dev Jul 19, 2020
@viceice viceice deleted the fix/753-function-constructor branch July 21, 2020 04:15
@viceice
Copy link
Contributor Author

viceice commented Jul 21, 2020

Had not realized that all these UUid classes were only in Tests, until I start adding what I suggested.
Doing it in a different PR then.
You should write a wiki entry with this UUid example and how to get to it, would be very helpful for many.

I've started here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FunctionInstance no longer extendable
3 participants