-
Notifications
You must be signed in to change notification settings - Fork 54
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
[Contexts] introduce contexts to make params list for all kinds of extensions smaller and easier extendable #363
[Contexts] introduce contexts to make params list for all kinds of extensions smaller and easier extendable #363
Conversation
dpfaffenbauer
commented
Jul 13, 2022
Q | A |
---|---|
Bug fix? | no |
New feature? | yes |
BC breaks? | YES |
Deprecations? | no |
…tensions smaller and easier extendable
LGTM! Just kidding, I'll need to look at it a bit closely. I'll have time next Monday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a huge change and I'd love it if unrelated CS reformatting was not a part of it.
Would it be possible to have a root ContextInterface
and have the specific interfaces extend it, and also to push common stuff there? It might mean we'll be able to have less interfaces since some of them might be exactly the same, we can then find a better naming instead of forcing <Usecase>Interface
format.
DataDefinitionInterface $definition, | ||
array $params, | ||
array $dataRow, | ||
?ImportDataSetInterface $dataSet, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Import
here since it's not import specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly, it can be used for exports too.
src/DataDefinitionsBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
src/DataDefinitionsBundle/Interpreter/AssetByPathInterpreter.php
Outdated
Show resolved
Hide resolved
src/DataDefinitionsBundle/ProcessManager/ArtifactGenerationProviderInterface.php
Show resolved
Hide resolved
add basic context rename setter to mapping
…/contexts # Conflicts: # src/DataDefinitionsBundle/Resources/config/services.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it looks good to me, but since it's so huge, I could obviously be wrong. Let's merge here and we'll fix the issues as they come.