Process payload synchronously in debug mode

[theofidry]

Closes #7.

I applied the same log as for the rest of amphp. I'm not sure however that we really need a dedicated mode for parallel-functions so I sticked to AMP_DEBUG for now.

[kelunik]

Didn't you want to put the code into parallel() instead?

I don't like abusing AMP_DEBUG for that, as it's a change in behavior that can result in code no longer working properly. Instead, this should have a dedicated flag IMO.

Maybe we should just use a constant for this one instead of making the behavior togglable? \getenv has some overhead, but maybe we can just ignore that.

[theofidry]

Yeah you're right. Updated :)

[theofidry]

Also unrelated side notes:

• I find the code a bit hard to read with all those leading \. IMO it would be better to go for use statements rather than FQ forms all the time
• The CS seems a bit weird sometimes, especially the opening bracket on the same line which I rarely see in PHP. But well, it's just CS in the end...
• I'm not sure to get why you have Internal directories: isn't @internal enough?
• Have you considered to go mono-repo? It seems there is a lot of sub-proects under amphp but managing all of them separately looks tough

Otherwise again, thanks for the package that is working relatively well so far :)

[bwoebi]

Regarding the mono-repo: we still want to respect semver though. If we end up releasing a new major of e.g. amphp/parallel, we do not have to release majors for all the other code. Mono-repos sure are great for applications, not so much for libraries, I think. If I happen to be wrong, then please show me :-)

[staabm]

When you want monorepos and have separate lifecycles (but e.g. share the major version with all components) you need a monorepo (single source of true) and and repo per component (in which you create the release tags), which can be one way synched via a automatism (symfony has this kind of structure).

otherwise it doesnt help that much, I agree

[kelunik]

• \ is mainly getting used to it. After that, it's easier to handle it inline than to have a list of imports where a function may or may not be imported. That way it's easily visible.
• The code style is PSR-2 except for the { for methods and classes. Anything else that's strange?
• Internal ensures also people without IDEs notice they shouldn't use these things.
• Managing the dependencies is pretty fine as long as there's no new major version of Amp.

@staabm As far as I'm aware, Symfony uses separate repositories, but uses the same tags everywhere, no? So that wouldn't solve anything.

[theofidry]

\ is mainly getting used to it. After that, it's easier to handle it inline than to have a list of imports where a function may or may not be imported. That way it's easily visible.

I must say I'm having hard time with those and I'm way more comfortable with use statements which are folded by default so it doesn't matter if they are big or not.

The code style is PSR-2 except for the { for methods and classes. Anything else that's strange?

That's the main thing that strikes me. But tbh it's not that bad I'm not against the style itself, it's more that combined with the previous point that made it a bit more cryptic than it needed to be IMO.

Internal ensures also people without IDEs notice they shouldn't use these things.

Fair enough. I tend to not worry too much about it and I don't like much to change the namespace depending of wether or not things are internal but I see the point.

You're also right about the mono-repo. I just think it's easier to manage but if you prefer that granular control multiple repositories are fine. I do find them as good for libs than applications though.

Well of that said, sorry for the distraction/noise :) Is there anything you would like me to change for the PR or it's all good?

[kelunik]

@theofidry I'm not 100% sold on the approach, yet. Another approach can be implementing a SyncWorker for amphp/parallel.

[theofidry]

That's true but I found it way more complex (at least to me to write it — that was my first attempt) and really wouldn't help IMO. What I would like to debug here is the callable which is not doable as soon as it is serialized. Debugging the serialization can already be done already via a breakpoint or else. If the code works synchronously with that approach and the serialization is correct, then only the parallel execution is left as a potential issue. But I however think this may warrant another approach for debugging it.

TBH the whole thing could be avoided if we were not losing the breakpoint during the serialization, but I just don't know how to do that...

[kelunik]

@theofidry Excellent observation! Have you tried using xdebug_break() yet?

[theofidry]

Nope, will give it a go. I didn’t know about that one :P

…

On Fri 2 Mar 2018 at 21:41, Niklas Keller ***@***.***> wrote: @theofidry <https://github.com/theofidry> Excellent observation! Have you tried using xdebug_break() yet? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE76gcuOIjZntqWVHLdkJCcadoh-hqy_ks5taa5pgaJpZM4SYccv> .

[theofidry]

Tried it but doesn't seem to work. You can give it a shot as well. I putted the breakpoint/xdebug_break() call there and then you can run bin/box build

[theofidry]

@kelunik any thoughts on this? I appreciate the solution is not ideal, but it doesn't look like it's something we can fix at xdebug level :/

[kelunik]

@theofidry The current patch is wrong. The returned callable must always return a promise. Further, I don't like having a constant decide this, as that requires writing additional code for debugging. I guess we also need a better name for it, but I don't have a good suggestion, yet.

[theofidry]

As we couldn't find a satisfying implementation I'll be closing this one. Meanwhile I'm using a condition in Box:

return is_parallel_processing_enabled() && false === ($this->scoper instanceof NullScoper)
            ? wait(parallelMap($files, $processFile))
            : array_map($processFile, $files)
        ;

Which is good enough for me and is a dev mode that is from Box instead of Amp