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

Move Context to the constructor #5

Closed
gabesullice opened this issue Mar 7, 2018 · 8 comments
Closed

Move Context to the constructor #5

gabesullice opened this issue Mar 7, 2018 · 8 comments

Comments

@gabesullice
Copy link

gabesullice commented Mar 7, 2018

Transformations should be dead simple and immutable. A Context means that the same data might be transformed differently if the "context" is different, which is just an arbitrary key-value store. That'll be bug prone. If you need to perform different transformations, just construct different transformation objects.

If you need to transform a sub-property of a larger object, based on some value elsewhere in the document, I think you should just be doing the transformation on a higher-level property. E.g. have a transformer on a higher-level property that gets the context it needs, constructs the lower level transforms, then does that transform on the lower level property.

@e0ipso
Copy link
Owner

e0ipso commented Mar 8, 2018

For now I'd rather keep the contex where it is. The reason for that is that if we keep this state out of the object you can put these data adaptors into service containers pretty easily.

If you need to perform different transformations, just construct different transformation objects.

That is spot on. However you can imagine how many times you cannot determine the necessary transformations beforehand, but you need to inspect your surroundings at run time.

@gabesullice
Copy link
Author

Isn't that what factories are for?

@e0ipso
Copy link
Owner

e0ipso commented Mar 8, 2018

In this case you only move the $context passing around. What is the benefit of setting it in the constructor or passing it during transform?

@gabesullice
Copy link
Author

Because then at a higher level, I can just deal with transformation objects and have no understanding of their requirements.

Like in the versioning manager, I could just ask a transformation manager for all transformations by their ID. Then I need only "apply" the transforms in the appropriate order. I don't need to know about each transformations need for context.

At some point, this abstraction will need to exist, it's just whether it's in this library or another one.

@e0ipso
Copy link
Owner

e0ipso commented Mar 8, 2018

You can do just that. The $contex argument is optional.

@gabesullice
Copy link
Author

One way I could see this working is annotating all transformations. Transforms that need context would specify a factory in their annotation.

When I ask a transformation manager for a transformation with a factory, it won't give me the transformation directly, it'll give me a special transformation that is injected with the factory. Whenever I call transform on it, it will instantiate the needed transform before applying it.

@gabesullice
Copy link
Author

gabesullice commented Mar 8, 2018

You can do just that. The $contex argument is optional.

But transformations like the ones in the README would break, no? Then I'd need to somehow know which transformations need a context and which don't. Perhaps I'm just not seeing the necessary use-case for this.

@e0ipso
Copy link
Owner

e0ipso commented May 19, 2021

This is quite similar to what we ended up doing in JsonRpc :-)

@e0ipso e0ipso closed this as completed May 19, 2021
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

No branches or pull requests

2 participants