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

Parameterhandler add json support #104

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

benjamin-carl
Copy link

Hi Christophe,

I've been using your library in a Symfony context for a longer time, loved it cause of its simplicity - so btw thanks for it! I required something similar in a different context and with JSON support. So I've forked your project and added support for JSON while keeping everything else as untouched as possible ;)

I've re-adjusted some directoy layouts to provide a similar test stcuture for example and fulfilled all existing tests for the JSON part.

I'm pretty sure that it will be very helpful for other developers as well and so I hope you're willed to merge my addition. Yes I know - it's a huge PR and contains a lot of changes - but all tests running successful. So lets see the Travis build results for other PHP versions ...

Ben

Benjamin Carl added 30 commits September 10, 2016 08:19
@OskarStark
Copy link

WOW, what a huge PR 👍

@clickalicious i think you should provide an UPGRADE path, because there are some BC breaks in your PR

@benjamin-carl
Copy link
Author

Can you please explain what UPGRADE path you refer to @OskarStark.

Indeed I've missed to look at the existing PR's and a merge would break those. A possbile upgrade path would contain the merge of existing accepted PRs and I readjust some parts of my work afterwards.

So the UPGRADE path should contain the merge of existing PRs and i realign everything based on the result. Currently I can't imagine how many of the 17 open PRs are going to be merged into the codebase.

From outside view my PR does not break any existing functionality but providing an additional format.
I changed the way how the ScriptHandler calls the YamlProcessor or JsonProcessor. This way contains the creation of instances in a loop so it have some negative but from my pov irelevant impact on performance. In result it would be possible to mix dist file formats in one project/execution.

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.

None yet

2 participants