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

Implement NeonizerExtension::validate #2

Closed
wants to merge 3 commits into from

Conversation

2 participants
@jiripudil
Copy link

commented Jan 1, 2018

closes #1

@f3l1x

This comment has been minimized.

Copy link
Member

commented Jan 1, 2018

Thanks for the PR. I will test it and discuss it with @benijo.

use Contributte\Neonizer\Config\FileConfig;
use Contributte\Neonizer\Decoder\IDecoderFactory;
class FileLoader

This comment has been minimized.

Copy link
@f3l1x

f3l1x Jan 3, 2018

Member

Well, why do you add this class? I mean, there's no interface, there's no any other implementation. Maybe, I would do it more simple and keep it the old way, WDYT?

This comment has been minimized.

Copy link
@jiripudil

jiripudil Jan 3, 2018

Author

To prevent code duplication; this file loading bit is now used not only by FileProcessor, where the code originally was, but also by FileValidator, so I extracted it into a separate class. I'm open to ideas how to do it any other old way.

I personally tend not to introduce interfaces when there is only one implementation because it's unnecessary and can be easily abstracted later when the need arises. But I'm not against the idea, if you'd like me to add an interface for this class.

@f3l1x

This comment has been minimized.

Copy link
Member

commented Mar 24, 2018

Sorry @jiripudil it takes so long time. I'm not sure how to say, but I'm not sure if it's should be implement it in this way. I mean, it's not wrong and it deserve to be merged. There's some tiny detail, FileProcessor requires IEncoderFactory and IDecoderFactory at this time. You wrap IDecoderFactory into FileLoader. So new dependencies will be (IEncoderFactory $d1, FileLoader $d2) and looks strange for me. Where's decoder, is it still really needed? It's in FileLoader, I see... Do you get my point?

What about move FileLoader into Decoder class? And DecoderFactory will be responsible to setup proper FileLoader. For example:

$dc = new DecoderFactor();
$dc->setLoader(new FileLoader());
$dc->create()

jiripudil added some commits Jan 1, 2018

@jiripudil

This comment has been minimized.

Copy link
Author

commented Apr 2, 2018

What about move FileLoader into Decoder class?

Frankly, I don't like that idea. The code would be where it was prior to this PR, but with an object wrapper over file_get_contents.

FileLoader was just a result of extracting a piece of code that would be unnecessarily duplicated. I understand you are uneasy about it, and I don't want to spend too much time inventing abstractions, so I've just reverted that part of the commit.

@f3l1x f3l1x closed this in 3337853 Apr 7, 2018

@f3l1x

This comment has been minimized.

Copy link
Member

commented Apr 7, 2018

Thank you @jiripudil. I very appreciate your time on this PR. I've rewritte internal architecture and used some ideas/code from your PR to close it. :-) Happy to test it.

@f3l1x f3l1x added this to the v0.2 milestone Apr 7, 2018

@jiripudil jiripudil deleted the jiripudil:1-validate-config branch Apr 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.