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

Updated graphViz to work on OS X #67

Merged
merged 2 commits into from
Aug 5, 2013
Merged

Updated graphViz to work on OS X #67

merged 2 commits into from
Aug 5, 2013

Conversation

onigoetz
Copy link
Contributor

@onigoetz onigoetz commented Aug 2, 2013

on Mac OS X the way to open a file with the default handler is "open"

I added a switch to reflect this

on Mac OS X the way to open a file with the default handler is "open"

I added a switch to reflect this
@clue
Copy link
Member

clue commented Aug 2, 2013

Thanks for the PR! I don't have a Mac to test against, but according to the manpages this addition looks good 👍

May I ask you to re-format it as an if-elseif-else-statement instead and format according to PSR-2?

Imho at some point in the future we should consider removing this logic from this library and check if we can add an external dependency. In the meantime, I'd like to merge this PR as-is :)

@clemens-tolboom
Copy link
Collaborator

I should test this as I have a mac.

Do we have a guide how to hit this code?

@onigoetz
Copy link
Contributor Author

onigoetz commented Aug 2, 2013

Hi,

okay I'll reformat it.

@clemens-tolboom I tested it using https://github.com/clue/graph-composer it worked perfectly after the change

@clue
Copy link
Member

clue commented Aug 2, 2013

Do we have a guide how to hit this code?

Minimal test code:

$graph = new Graph();
$v1 = $graph->createVertex(1);

$graphviz = new GraphViz($graph);
$graphviz->display();

Should be executed via CLI (php test.php).

If your "image viewer" pops up, everything works fine.

@clue
Copy link
Member

clue commented Aug 3, 2013

Thanks for the update! Code looks good to me 👍

Waiting for confirmation from @clemens-tolboom.

@clemens-tolboom
Copy link
Collaborator

I can confirm this PR works. Nice fix.

I learned about https://help.github.com/articles/checking-out-pull-requests-locally and needed a small test tweak. Not sure why :(

<?php
require 'vendor/autoload.php';

$graph = new Fhaculty\Graph\Graph();
$v1 = $graph->createVertex(1);

$graphviz = new Fhaculty\Graph\GraphViz($graph);
$graphviz->display();

clue added a commit that referenced this pull request Aug 5, 2013
Updated graphViz to work on OS X
@clue clue merged commit a9660f5 into graphp:master Aug 5, 2013
@clue
Copy link
Member

clue commented Aug 5, 2013

Thanks for the confirmation, code merged!

Yeah, my test case was written under the assumption that the required imports are present :) By using fully-qualified class names, you no longer need those imports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants