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

handle shebang #93

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mykiwi
Copy link

commented Apr 23, 2019

#92

Show resolved Hide resolved src/SensioLabs/Melody/Resource/LocalResource.php Outdated
Show resolved Hide resolved src/SensioLabs/Melody/Handler/FileHandler.php Outdated
if ($script->getResource() instanceof LocalResource) {
$bootstrap = $this->getLocalBootstrap($script->getResource());
$resource = $script->getResource();
if ($resource instanceof LocalResource && !$resource->hadShebang()) {

This comment has been minimized.

Copy link
@jderusse

jderusse Apr 24, 2019

Contributor

Looking at code, I wonder if we shouldn't just always use the getRemoteBootstrap, that would avoid the hasShebang method

This comment has been minimized.

Copy link
@lyrixx

lyrixx Apr 29, 2019

Member

I agree with @jderusse ; We could simplify the code here

@staabm

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Any chance to get a unit test ?

@mykiwi

This comment has been minimized.

Copy link
Author

commented Apr 24, 2019

Yes, once the code will stop changing

mykiwi added some commits Apr 24, 2019

@mykiwi mykiwi changed the title [WIP] handle shebang handle shebang Apr 24, 2019

mykiwi added some commits Apr 24, 2019

@mykiwi

This comment has been minimized.

Copy link
Author

commented Apr 26, 2019

I think this PR is ready.
Should I do something else? (like remove the getLocalBootstrap part ?)

@staabm

staabm approved these changes Apr 26, 2019

Copy link
Contributor

left a comment

lgtm

Show resolved Hide resolved src/SensioLabs/Melody/Handler/FileHandler.php Outdated
if ($script->getResource() instanceof LocalResource) {
$bootstrap = $this->getLocalBootstrap($script->getResource());
$resource = $script->getResource();
if ($resource instanceof LocalResource && !$resource->hadShebang()) {

This comment has been minimized.

Copy link
@lyrixx

lyrixx Apr 29, 2019

Member

I agree with @jderusse ; We could simplify the code here

@mykiwi

This comment has been minimized.

Copy link
Author

commented May 1, 2019

Done
What do you think @jderusse @lyrixx ?

@jderusse
Copy link
Contributor

left a comment

A small comment

@mykiwi

This comment has been minimized.

Copy link
Author

commented May 8, 2019

up ?

@lyrixx

lyrixx approved these changes May 9, 2019

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.