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

Add dry_run option #35

Merged
merged 10 commits into from Jun 16, 2017

Conversation

Projects
None yet
4 participants
@jjatria
Contributor

jjatria commented Apr 3, 2017

This is a work in progress (comments welcome).

Here's one possibility for the dry_run option requested in #24. This first version uses Graph::Easy for the dump, which looks wonderful, but seems a bit wasteful, since we are loading an entirely new graph module only to dump the tree.

The benefit of using Graph::Easy, though, is that it already has many different ways to dump graphs. Applying the new option to the Test1::Step::CombineFiles final step, the plot looks like these:

Using as_ascii:

+---------------------------+     +--------------------------+     +-----------------------+
| Test1::Step::CombineFiles | --> | Test1::Step::UpdateFiles | --> | Test1::Step::CreateA1 |
+---------------------------+     +--------------------------+     +-----------------------+
                                    |
                                    |
                                    v
                                  +--------------------------+
                                  |  Test1::Step::CreateA2   |
                                  +--------------------------+

Using as_boxart, which uses wide characters:

┌───────────────────────────┐     ┌──────────────────────────┐     ┌───────────────────────┐
│ Test1::Step::CombineFiles │ ──> │ Test1::Step::UpdateFiles │ ──> │ Test1::Step::CreateA1 │
└───────────────────────────┘     └──────────────────────────┘     └───────────────────────┘
                                    │
                                    │
                                    ∨
                                  ┌──────────────────────────┐
                                  │  Test1::Step::CreateA2   │
                                  └──────────────────────────┘

Using as_txt, which is by far the simplest (and therefore the simplest to emulate without Graph::Easy):

[ Test1::Step::CombineFiles ] --> [ Test1::Step::UpdateFiles ]
[ Test1::Step::UpdateFiles ] --> [ Test1::Step::CreateA1 ]
[ Test1::Step::UpdateFiles ] --> [ Test1::Step::CreateA2 ]

The last one has the benefit that it looks like the one that would scale the best as the number of steps increases.

The other thing I'm not so happy about is that the runner is now doing the printing, which feels wrong. The printing could be passed to the logger, but then the only real alternative would be a list as the one you get from as_txt. And even then, I don't know if the logger is the right place for it either.

@oalders

This looks really good. I've added a few comments. Would it be possible to add some kind of a test for this as well?

Show outdated Hide outdated lib/Stepford/Runner.pm
my $root_graph
= $self->_make_root_graph_builder( $final_steps, $config )->graph;
if ( $self->jobs > 1 ) {
if ($dry_run) {
() = print $self->_dump_steps($root_graph);

This comment has been minimized.

@oalders

oalders Apr 4, 2017

Member

Is this to get around a Perl::Critic complaint?

@oalders

oalders Apr 4, 2017

Member

Is this to get around a Perl::Critic complaint?

This comment has been minimized.

@jjatria

jjatria Apr 4, 2017

Contributor

Yeah. I guess I should have turned it off, but at the time this seemed easier.

@jjatria

jjatria Apr 4, 2017

Contributor

Yeah. I guess I should have turned it off, but at the time this seemed easier.

Show outdated Hide outdated lib/Stepford/Runner.pm
$root->traverse(
sub {
my $self = shift;

This comment has been minimized.

@oalders

oalders Apr 4, 2017

Member

Maybe we should call this something other than $self to make it less confusing.

@oalders

oalders Apr 4, 2017

Member

Maybe we should call this something other than $self to make it less confusing.

Show outdated Hide outdated lib/Stepford/Runner.pm
sub {
my $self = shift;
if ( $self->step_class ne 'Stepford::FinalStep' ) {
$graph->add_edge_once( $self->step_class => $_->step_class )

This comment has been minimized.

@oalders

oalders Apr 4, 2017

Member

The postfix foreach is dangerous here as any modification of $_ will modify $self->{_children_graphs}.

@oalders

oalders Apr 4, 2017

Member

The postfix foreach is dangerous here as any modification of $_ will modify $self->{_children_graphs}.

This comment has been minimized.

@jjatria

jjatria Apr 4, 2017

Contributor

Yikes. I'll change that then.

@jjatria

jjatria Apr 4, 2017

Contributor

Yikes. I'll change that then.

Show outdated Hide outdated lib/Stepford/Runner.pm
# Requires binmode STDOUT, ':utf8';
# return $graph->as_boxart;
}

This comment has been minimized.

@oalders

oalders Apr 4, 2017

Member

We should remove the commented code. The boxart looks nicest of the three options. How about moving this logic into Stepford::Graph and then replacing as_string() with what you've written? That eliminates the need to access a private variable from another class and compartmentalizes this code nicely.

So, the dry run would just call as_string on the Stepford::Graph object.

@oalders

oalders Apr 4, 2017

Member

We should remove the commented code. The boxart looks nicest of the three options. How about moving this logic into Stepford::Graph and then replacing as_string() with what you've written? That eliminates the need to access a private variable from another class and compartmentalizes this code nicely.

So, the dry run would just call as_string on the Stepford::Graph object.

This comment has been minimized.

@jjatria

jjatria Apr 4, 2017

Contributor

Sounds good. The comments were there just as a reminder (to me, mostly) of the different possibilities I was trying out. I'll move this to Stepford::Graph and squash those changes into this commit.

@jjatria

jjatria Apr 4, 2017

Contributor

Sounds good. The comments were there just as a reminder (to me, mostly) of the different possibilities I was trying out. I'll move this to Stepford::Graph and squash those changes into this commit.

@jjatria

This comment has been minimized.

Show comment
Hide comment
@jjatria

jjatria Apr 4, 2017

Contributor

Thanks! Glad you like it.

Would it be possible to add some kind of a test for this as well?

Absolutely. Let me put some together and I'll add them to the branch.

Contributor

jjatria commented Apr 4, 2017

Thanks! Glad you like it.

Would it be possible to add some kind of a test for this as well?

Absolutely. Let me put some together and I'll add them to the branch.

jjatria added some commits Apr 3, 2017

Add tests for dry run
The Runner.t test was also modified to use Graph::Easy to generate
the expected values of the step plan.
@jjatria

This comment has been minimized.

Show comment
Hide comment
@jjatria

jjatria Apr 13, 2017

Contributor

Do you have any updates on this, @oalders? I think I've made the changes you requested, and I've made sure that tests pass.

Is there anything holding this back? Thanks!

Contributor

jjatria commented Apr 13, 2017

Do you have any updates on this, @oalders? I think I've made the changes you requested, and I've made sure that tests pass.

Is there anything holding this back? Thanks!

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Apr 21, 2017

Member

This looks good to me. This is the diff I get for cpanfile:

--- a/cpanfile
+++ b/cpanfile
@@ -1,5 +1,5 @@
 requires "Carp" => "0";
-requires "File::Temp" => "0";
+requires "Graph::Easy" => "0.76";
 requires "List::AllUtils" => "0";
 requires "Log::Dispatch" => "0";
 requires "Log::Dispatch::Null" => "0";
@@ -16,7 +16,6 @@ requires "MooseX::Types::Moose" => "0";
 requires "MooseX::Types::Path::Class" => "0";
 requires "Parallel::ForkManager" => "0";
 requires "Path::Class" => "0";
-requires "Scalar::Util" => "0";
 requires "Scope::Guard" => "0";
 requires "Throwable::Error" => "0";
 requires "Time::HiRes" => "1.9726";
@@ -29,15 +28,14 @@ requires "warnings" => "0";
 recommends "Memory::Stats" => "0";

 on 'test' => sub {
+  requires "Capture::Tiny" => "0";
   requires "ExtUtils::MakeMaker" => "0";
-  requires "File::Copy" => "0";
   requires "File::Spec" => "0";
-  requires "IPC::Signal" => "0";
+  requires "Graph::Easy" => "0.76";
   requires "Log::Dispatch::Array" => "0";
   requires "Test::Differences" => "0";
   requires "Test::Fatal" => "0";
   requires "Test::More" => "0.96";
-  requires "Test::Requires" => "0";
   requires "autodie" => "0";
   requires "lib" => "0";
 };
@@ -50,6 +48,10 @@ on 'configure' => sub {
   requires "ExtUtils::MakeMaker" => "0";
 };

+on 'configure' => sub {
+  suggests "JSON::PP" => "2.27300";
+};
+
 on 'develop' => sub {
   requires "Code::TidyAll::Plugin::Test::Vars" => "0.02";
   requires "Devel::Hide" => "0";

@oschwald what do you think?

Member

oalders commented Apr 21, 2017

This looks good to me. This is the diff I get for cpanfile:

--- a/cpanfile
+++ b/cpanfile
@@ -1,5 +1,5 @@
 requires "Carp" => "0";
-requires "File::Temp" => "0";
+requires "Graph::Easy" => "0.76";
 requires "List::AllUtils" => "0";
 requires "Log::Dispatch" => "0";
 requires "Log::Dispatch::Null" => "0";
@@ -16,7 +16,6 @@ requires "MooseX::Types::Moose" => "0";
 requires "MooseX::Types::Path::Class" => "0";
 requires "Parallel::ForkManager" => "0";
 requires "Path::Class" => "0";
-requires "Scalar::Util" => "0";
 requires "Scope::Guard" => "0";
 requires "Throwable::Error" => "0";
 requires "Time::HiRes" => "1.9726";
@@ -29,15 +28,14 @@ requires "warnings" => "0";
 recommends "Memory::Stats" => "0";

 on 'test' => sub {
+  requires "Capture::Tiny" => "0";
   requires "ExtUtils::MakeMaker" => "0";
-  requires "File::Copy" => "0";
   requires "File::Spec" => "0";
-  requires "IPC::Signal" => "0";
+  requires "Graph::Easy" => "0.76";
   requires "Log::Dispatch::Array" => "0";
   requires "Test::Differences" => "0";
   requires "Test::Fatal" => "0";
   requires "Test::More" => "0.96";
-  requires "Test::Requires" => "0";
   requires "autodie" => "0";
   requires "lib" => "0";
 };
@@ -50,6 +48,10 @@ on 'configure' => sub {
   requires "ExtUtils::MakeMaker" => "0";
 };

+on 'configure' => sub {
+  suggests "JSON::PP" => "2.27300";
+};
+
 on 'develop' => sub {
   requires "Code::TidyAll::Plugin::Test::Vars" => "0.02";
   requires "Devel::Hide" => "0";

@oschwald what do you think?

Show outdated Hide outdated lib/Stepford/Graph.pm
sub {
my $node = shift;
if ( $node->step_class ne 'Stepford::FinalStep' ) {
foreach ( @{ $node->{_children_graphs} } ) {

This comment has been minimized.

@oschwald

oschwald Apr 21, 2017

Member

You are still using the topic variable ($_) here. It is subject to clobbering if something in add_edge_once or step_classes modifies it without localizing it first.

Also, please use the accessor for _children_graphs.

@oschwald

oschwald Apr 21, 2017

Member

You are still using the topic variable ($_) here. It is subject to clobbering if something in add_edge_once or step_classes modifies it without localizing it first.

Also, please use the accessor for _children_graphs.

This comment has been minimized.

@jjatria

jjatria Apr 21, 2017

Contributor

Will do. I thought the problem was with the postfix foreach

@jjatria

jjatria Apr 21, 2017

Contributor

Will do. I thought the problem was with the postfix foreach

This comment has been minimized.

@jjatria

jjatria Apr 21, 2017

Contributor

Addressed in 8cdbc58

@jjatria

jjatria Apr 21, 2017

Contributor

Addressed in 8cdbc58

Show outdated Hide outdated lib/Stepford/Graph.pm
q{},
map { $_->as_string( $depth + 1 ) } @{ $self->_children_graphs }
require Graph::Easy;
my $graph = Graph::Easy->new;

This comment has been minimized.

@oschwald

oschwald Apr 21, 2017

Member

I tested this out. The as_txt style seems like it would be trivial to reproduce without the module, probably simpler than the current output that is being replaced. I didn't find the output particularly helpful on a large graph, but the current output isn't that great either.

Assuming we continue using the module, we should just use it at the top as this will be loaded whenever verbose is used and preloading it could detect issues sooner.

@oschwald

oschwald Apr 21, 2017

Member

I tested this out. The as_txt style seems like it would be trivial to reproduce without the module, probably simpler than the current output that is being replaced. I didn't find the output particularly helpful on a large graph, but the current output isn't that great either.

Assuming we continue using the module, we should just use it at the top as this will be loaded whenever verbose is used and preloading it could detect issues sooner.

This comment has been minimized.

@jjatria

jjatria Apr 21, 2017

Contributor

Sure. Graph::Easy has no dependencies, though, so I thought that instead of doing things by hand it might be simpler to just outsource it.
As for the format itself, I agree it does not in itself seem like such an improvement. But it has the benefit that it can then be passed as is back to Graph::Easy::Parser to generate all sorts of displays, not just text based.
Do you want me to remove the module, or move it to the top?

@jjatria

jjatria Apr 21, 2017

Contributor

Sure. Graph::Easy has no dependencies, though, so I thought that instead of doing things by hand it might be simpler to just outsource it.
As for the format itself, I agree it does not in itself seem like such an improvement. But it has the benefit that it can then be passed as is back to Graph::Easy::Parser to generate all sorts of displays, not just text based.
Do you want me to remove the module, or move it to the top?

This comment has been minimized.

@jjatria

jjatria Apr 21, 2017

Contributor

Addressed in 401803d

@jjatria

jjatria Apr 21, 2017

Contributor

Addressed in 401803d

Show outdated Hide outdated lib/Stepford/Runner.pm
if ( $self->jobs > 1 ) {
if ($dry_run) {
## no critic (InputOutput::RequireCheckedSyscalls)
print $root_graph->as_string;

This comment has been minimized.

@oalders

oalders Apr 21, 2017

Member

Could we add a parameter for the output format of the graph? If we could specify boxart (which would map to as_boxart, ascii maps to as_ascii etc, that would make this feature a lot more flexible. The param could accept a Str and then prefix it with as_ to get the appropriate method name. This exposes the Graph::Easy API, but since this is for debugging, that seems fine to me.

@oalders

oalders Apr 21, 2017

Member

Could we add a parameter for the output format of the graph? If we could specify boxart (which would map to as_boxart, ascii maps to as_ascii etc, that would make this feature a lot more flexible. The param could accept a Str and then prefix it with as_ to get the appropriate method name. This exposes the Graph::Easy API, but since this is for debugging, that seems fine to me.

This comment has been minimized.

@jjatria

jjatria Apr 21, 2017

Contributor

We could take an Enum instead of a Str, and make sure that the format is one of a set.

Alternatively, I can add a check with can to make sure that the method exists. If no corresponding method is found, it could issue a warning and fall back to as_txt.

@jjatria

jjatria Apr 21, 2017

Contributor

We could take an Enum instead of a Str, and make sure that the format is one of a set.

Alternatively, I can add a check with can to make sure that the method exists. If no corresponding method is found, it could issue a warning and fall back to as_txt.

This comment has been minimized.

@oalders

oalders Apr 21, 2017

Member

The enum sounds good to me.

@oalders

oalders Apr 21, 2017

Member

The enum sounds good to me.

This comment has been minimized.

@oalders

oalders Apr 21, 2017

Member

We should also make sure this is documented in the Pod.

@oalders

oalders Apr 21, 2017

Member

We should also make sure this is documented in the Pod.

This comment has been minimized.

@jjatria

jjatria Apr 21, 2017

Contributor

Will do. What should I do with the mention of dry runs in the Future Features section? It's the only item. Should I remove the entire section?

@jjatria

jjatria Apr 21, 2017

Contributor

Will do. What should I do with the mention of dry runs in the Future Features section? It's the only item. Should I remove the entire section?

This comment has been minimized.

@oalders

oalders Apr 21, 2017

Member

Thanks!

Should I remove the entire section

Please! Once we've documented dry run elsewhere, there's no point in having the empty section. :)

@oalders

oalders Apr 21, 2017

Member

Thanks!

Should I remove the entire section

Please! Once we've documented dry run elsewhere, there's no point in having the empty section. :)

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Apr 21, 2017

Member

Tiny issue, but the use module list is alpha sorted. :)

Tiny issue, but the use module list is alpha sorted. :)

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Apr 21, 2017

Member

👍

👍

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders May 17, 2017

Member

I tested the various output options. I get:


dry run with mode: none


dry run with mode: txt

[ DryTest::Step::D ] --> [ DryTest::Step::B ]
[ DryTest::Step::D ] --> [ DryTest::Step::C ]
[ DryTest::Step::B ] --> [ DryTest::Step::A ]
[ DryTest::Step::C ] --> [ DryTest::Step::B ]

dry run with mode: boxart


  ┌─────────────────────────────────────────────────┐
  │                                                 ∨
┌──────────────────┐     ┌──────────────────┐     ┌──────────────────┐     ┌──────────────────┐
│ DryTest::Step::D │ ──> │ DryTest::Step::C │ ──> │ DryTest::Step::B │ ──> │ DryTest::Step::A │
└──────────────────┘     └──────────────────┘     └──────────────────┘     └──────────────────┘

dry run with mode: ascii


  +-------------------------------------------------+
  |                                                 v
+------------------+     +------------------+     +------------------+     +------------------+
| DryTest::Step::D | --> | DryTest::Step::C | --> | DryTest::Step::B | --> | DryTest::Step::A |
+------------------+     +------------------+     +------------------+     +------------------+

dry run with mode: svg

<svg width="893.2" height="124.6" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<!-- Generated at Wed May 17 13:01:54 2017 by:
  Graph::Easy v0.26
  Graph::Easy::As_svg v0.26
-->

<title>Untitled graph</title>
<defs>
 <!-- open arrow -->
 <g id="ah" stroke-linecap="round" stroke-width="1">
  <line x1="-8" y1="-4" x2="1" y2="0" />
  <line x1="1" y1="0" x2="-8" y2="4" />
 </g>

 <!-- class definitions -->
 <style type="text/css"><![CDATA[
 .edge {
  font-size: 13px;
  stroke: black;
  text-align: center;
 }
 .graph {
  font-size: 16px;
  text-align: center;
 }
 .node {
  font-size: 16px;
  text-align: center;
 }
 ]]></style>
</defs>

<!-- graph background with border (mainly for printing) -->
<rect x="0.5" y="0.5" width="891.2" height="122.6" fill="white" stroke="white" />

<g id="100" class="edge">
 <!-- from DryTest::Step::B to DryTest::Step::A -->
 <!-- horizontal -->
 <line x1="648.84" y1="84.2" x2="704.16" y2="84.2" stroke="#000000" />
 <use stroke="#000000" xlink:href="#ah" x="705.16" y="84.2"/>
</g>

<g id="103" class="edge">
 <!-- from DryTest::Step::C to DryTest::Step::B -->
 <!-- horizontal -->
 <line x1="418.44" y1="84.2" x2="473.76" y2="84.2" stroke="#000000" />
 <use stroke="#000000" xlink:href="#ah" x="474.76" y="84.2"/>
</g>

<g id="105" class="edge">
 <!-- from DryTest::Step::D to DryTest::Step::B -->
 <!-- south/east corner -->
 <g stroke="#000000">
  <line x1="98.5" y1="38.9" x2="98.5" y2="55.4" />
  <line x1="98" y1="39.4" x2="180" y2="39.4" />
 </g>

 <!-- horizontal -->
 <line x1="180" y1="39.4" x2="481.8" y2="39.4" stroke="#000000" />

 <!-- south/west corner -->
 <g stroke="#000000">
  <line x1="561.8" y1="38.9" x2="561.8" y2="56.32" />
  <line x1="481.8" y1="39.4" x2="562.3" y2="39.4" />
 </g>
 <use stroke="#000000" xlink:href="#ah" transform="translate(561.8 57.32)rotate(90)"/>
</g>

<g id="107" class="edge">
 <!-- from DryTest::Step::D to DryTest::Step::C -->
 <!-- horizontal -->
 <line x1="187.04" y1="84.2" x2="242.36" y2="84.2" stroke="#000000" />
 <use stroke="#000000" xlink:href="#ah" x="243.36" y="84.2"/>
</g>

<g id="101" class="node">
 <!-- DryTest::Step::B, rect -->
 <rect fill="#ffffff" height="43.8" stroke="#000000" width="159" x="482.3" y="62.3" />
 <text x="561" y="90" style="font-family:serif" fill="#000000" text-anchor="middle">DryTest::Step::B</text>
</g>

<g id="102" class="node">
 <!-- DryTest::Step::A, rect -->
 <rect fill="#ffffff" height="43.8" stroke="#000000" width="162" x="712.7" y="62.3" />
 <text x="793" y="90" style="font-family:serif" fill="#000000" text-anchor="middle">DryTest::Step::A</text>
</g>

<g id="104" class="node">
 <!-- DryTest::Step::C, rect -->
 <rect fill="#ffffff" height="43.8" stroke="#000000" width="160" x="250.9" y="62.3" />
 <text x="330" y="90" style="font-family:serif" fill="#000000" text-anchor="middle">DryTest::Step::C</text>
</g>

<g id="106" class="node">
 <!-- DryTest::Step::D, rect -->
 <rect fill="#ffffff" height="43.8" stroke="#000000" width="162" x="17.5" y="62.3" />
 <text x="98" y="90" style="font-family:serif" fill="#000000" text-anchor="middle">DryTest::Step::D</text>
</g>

</svg>
dry run with mode: graphviz

digraph GRAPH_0 {

  // Generated by Graph::Easy 0.26 at Wed May 17 13:01:54 2017

  edge [ arrowhead=open ];
  graph [ rankdir=LR ];
  node [
    fillcolor=white,
    fontsize=11,
    shape=box,
    style=filled ];

  "DryTest::Step::B" -> "DryTest::Step::A" [ color="#000000" ]
  "DryTest::Step::C" -> "DryTest::Step::B" [ color="#000000" ]
  "DryTest::Step::D" -> "DryTest::Step::C" [ color="#000000" ]
  "DryTest::Step::D" -> "DryTest::Step::B" [ color="#000000" ]

}

dry run with mode: graphml

<?xml version="1.0" encoding="UTF-8"?>
<graphml xmlns="http://graphml.graphdrawing.org/xmlns"
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xsi:schemaLocation="http://graphml.graphdrawing.org/xmlns
     http://graphml.graphdrawing.org/xmlns/1.0/graphml.xsd">

  <!-- Created by Graph::Easy v0.26 at Wed May 17 13:01:55 2017 -->

  <graph id="G" edgedefault="directed">
    <node id="DryTest::Step::A">
    </node>
    <node id="DryTest::Step::B">
    </node>
    <node id="DryTest::Step::C">
    </node>
    <node id="DryTest::Step::D">
    </node>

    <edge source="DryTest::Step::B" target="DryTest::Step::A">
    </edge>
    <edge source="DryTest::Step::C" target="DryTest::Step::B">
    </edge>
    <edge source="DryTest::Step::D" target="DryTest::Step::B">
    </edge>
    <edge source="DryTest::Step::D" target="DryTest::Step::C">
    </edge>
  </graph>
</graphml>

dry run with mode: vcg

graph: {

  // Generated by Graph::Easy 0.26 at Wed May 17 13:01:55 2017

  node: { title: "DryTest::Step::A" }
  node: { title: "DryTest::Step::B" }
  node: { title: "DryTest::Step::C" }
  node: { title: "DryTest::Step::D" }

  edge: { sourcename: "DryTest::Step::B" targetname: "DryTest::Step::A" }
  edge: { sourcename: "DryTest::Step::C" targetname: "DryTest::Step::B" }
  edge: { sourcename: "DryTest::Step::D" targetname: "DryTest::Step::C" }
  edge: { sourcename: "DryTest::Step::D" targetname: "DryTest::Step::B" }

}

dry run with mode: gdl

graph: {

  // Generated by Graph::Easy 0.26 at Wed May 17 13:01:55 2017

  node: { title: "DryTest::Step::A" }
  node: { title: "DryTest::Step::B" }
  node: { title: "DryTest::Step::C" }
  node: { title: "DryTest::Step::D" }

  edge: { sourcename: "DryTest::Step::B" targetname: "DryTest::Step::A" }
  edge: { sourcename: "DryTest::Step::C" targetname: "DryTest::Step::B" }
  edge: { sourcename: "DryTest::Step::D" targetname: "DryTest::Step::C" }
  edge: { sourcename: "DryTest::Step::D" targetname: "DryTest::Step::B" }

}
Member

oalders commented May 17, 2017

I tested the various output options. I get:


dry run with mode: none


dry run with mode: txt

[ DryTest::Step::D ] --> [ DryTest::Step::B ]
[ DryTest::Step::D ] --> [ DryTest::Step::C ]
[ DryTest::Step::B ] --> [ DryTest::Step::A ]
[ DryTest::Step::C ] --> [ DryTest::Step::B ]

dry run with mode: boxart


  ┌─────────────────────────────────────────────────┐
  │                                                 ∨
┌──────────────────┐     ┌──────────────────┐     ┌──────────────────┐     ┌──────────────────┐
│ DryTest::Step::D │ ──> │ DryTest::Step::C │ ──> │ DryTest::Step::B │ ──> │ DryTest::Step::A │
└──────────────────┘     └──────────────────┘     └──────────────────┘     └──────────────────┘

dry run with mode: ascii


  +-------------------------------------------------+
  |                                                 v
+------------------+     +------------------+     +------------------+     +------------------+
| DryTest::Step::D | --> | DryTest::Step::C | --> | DryTest::Step::B | --> | DryTest::Step::A |
+------------------+     +------------------+     +------------------+     +------------------+

dry run with mode: svg

<svg width="893.2" height="124.6" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<!-- Generated at Wed May 17 13:01:54 2017 by:
  Graph::Easy v0.26
  Graph::Easy::As_svg v0.26
-->

<title>Untitled graph</title>
<defs>
 <!-- open arrow -->
 <g id="ah" stroke-linecap="round" stroke-width="1">
  <line x1="-8" y1="-4" x2="1" y2="0" />
  <line x1="1" y1="0" x2="-8" y2="4" />
 </g>

 <!-- class definitions -->
 <style type="text/css"><![CDATA[
 .edge {
  font-size: 13px;
  stroke: black;
  text-align: center;
 }
 .graph {
  font-size: 16px;
  text-align: center;
 }
 .node {
  font-size: 16px;
  text-align: center;
 }
 ]]></style>
</defs>

<!-- graph background with border (mainly for printing) -->
<rect x="0.5" y="0.5" width="891.2" height="122.6" fill="white" stroke="white" />

<g id="100" class="edge">
 <!-- from DryTest::Step::B to DryTest::Step::A -->
 <!-- horizontal -->
 <line x1="648.84" y1="84.2" x2="704.16" y2="84.2" stroke="#000000" />
 <use stroke="#000000" xlink:href="#ah" x="705.16" y="84.2"/>
</g>

<g id="103" class="edge">
 <!-- from DryTest::Step::C to DryTest::Step::B -->
 <!-- horizontal -->
 <line x1="418.44" y1="84.2" x2="473.76" y2="84.2" stroke="#000000" />
 <use stroke="#000000" xlink:href="#ah" x="474.76" y="84.2"/>
</g>

<g id="105" class="edge">
 <!-- from DryTest::Step::D to DryTest::Step::B -->
 <!-- south/east corner -->
 <g stroke="#000000">
  <line x1="98.5" y1="38.9" x2="98.5" y2="55.4" />
  <line x1="98" y1="39.4" x2="180" y2="39.4" />
 </g>

 <!-- horizontal -->
 <line x1="180" y1="39.4" x2="481.8" y2="39.4" stroke="#000000" />

 <!-- south/west corner -->
 <g stroke="#000000">
  <line x1="561.8" y1="38.9" x2="561.8" y2="56.32" />
  <line x1="481.8" y1="39.4" x2="562.3" y2="39.4" />
 </g>
 <use stroke="#000000" xlink:href="#ah" transform="translate(561.8 57.32)rotate(90)"/>
</g>

<g id="107" class="edge">
 <!-- from DryTest::Step::D to DryTest::Step::C -->
 <!-- horizontal -->
 <line x1="187.04" y1="84.2" x2="242.36" y2="84.2" stroke="#000000" />
 <use stroke="#000000" xlink:href="#ah" x="243.36" y="84.2"/>
</g>

<g id="101" class="node">
 <!-- DryTest::Step::B, rect -->
 <rect fill="#ffffff" height="43.8" stroke="#000000" width="159" x="482.3" y="62.3" />
 <text x="561" y="90" style="font-family:serif" fill="#000000" text-anchor="middle">DryTest::Step::B</text>
</g>

<g id="102" class="node">
 <!-- DryTest::Step::A, rect -->
 <rect fill="#ffffff" height="43.8" stroke="#000000" width="162" x="712.7" y="62.3" />
 <text x="793" y="90" style="font-family:serif" fill="#000000" text-anchor="middle">DryTest::Step::A</text>
</g>

<g id="104" class="node">
 <!-- DryTest::Step::C, rect -->
 <rect fill="#ffffff" height="43.8" stroke="#000000" width="160" x="250.9" y="62.3" />
 <text x="330" y="90" style="font-family:serif" fill="#000000" text-anchor="middle">DryTest::Step::C</text>
</g>

<g id="106" class="node">
 <!-- DryTest::Step::D, rect -->
 <rect fill="#ffffff" height="43.8" stroke="#000000" width="162" x="17.5" y="62.3" />
 <text x="98" y="90" style="font-family:serif" fill="#000000" text-anchor="middle">DryTest::Step::D</text>
</g>

</svg>
dry run with mode: graphviz

digraph GRAPH_0 {

  // Generated by Graph::Easy 0.26 at Wed May 17 13:01:54 2017

  edge [ arrowhead=open ];
  graph [ rankdir=LR ];
  node [
    fillcolor=white,
    fontsize=11,
    shape=box,
    style=filled ];

  "DryTest::Step::B" -> "DryTest::Step::A" [ color="#000000" ]
  "DryTest::Step::C" -> "DryTest::Step::B" [ color="#000000" ]
  "DryTest::Step::D" -> "DryTest::Step::C" [ color="#000000" ]
  "DryTest::Step::D" -> "DryTest::Step::B" [ color="#000000" ]

}

dry run with mode: graphml

<?xml version="1.0" encoding="UTF-8"?>
<graphml xmlns="http://graphml.graphdrawing.org/xmlns"
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xsi:schemaLocation="http://graphml.graphdrawing.org/xmlns
     http://graphml.graphdrawing.org/xmlns/1.0/graphml.xsd">

  <!-- Created by Graph::Easy v0.26 at Wed May 17 13:01:55 2017 -->

  <graph id="G" edgedefault="directed">
    <node id="DryTest::Step::A">
    </node>
    <node id="DryTest::Step::B">
    </node>
    <node id="DryTest::Step::C">
    </node>
    <node id="DryTest::Step::D">
    </node>

    <edge source="DryTest::Step::B" target="DryTest::Step::A">
    </edge>
    <edge source="DryTest::Step::C" target="DryTest::Step::B">
    </edge>
    <edge source="DryTest::Step::D" target="DryTest::Step::B">
    </edge>
    <edge source="DryTest::Step::D" target="DryTest::Step::C">
    </edge>
  </graph>
</graphml>

dry run with mode: vcg

graph: {

  // Generated by Graph::Easy 0.26 at Wed May 17 13:01:55 2017

  node: { title: "DryTest::Step::A" }
  node: { title: "DryTest::Step::B" }
  node: { title: "DryTest::Step::C" }
  node: { title: "DryTest::Step::D" }

  edge: { sourcename: "DryTest::Step::B" targetname: "DryTest::Step::A" }
  edge: { sourcename: "DryTest::Step::C" targetname: "DryTest::Step::B" }
  edge: { sourcename: "DryTest::Step::D" targetname: "DryTest::Step::C" }
  edge: { sourcename: "DryTest::Step::D" targetname: "DryTest::Step::B" }

}

dry run with mode: gdl

graph: {

  // Generated by Graph::Easy 0.26 at Wed May 17 13:01:55 2017

  node: { title: "DryTest::Step::A" }
  node: { title: "DryTest::Step::B" }
  node: { title: "DryTest::Step::C" }
  node: { title: "DryTest::Step::D" }

  edge: { sourcename: "DryTest::Step::B" targetname: "DryTest::Step::A" }
  edge: { sourcename: "DryTest::Step::C" targetname: "DryTest::Step::B" }
  edge: { sourcename: "DryTest::Step::D" targetname: "DryTest::Step::C" }
  edge: { sourcename: "DryTest::Step::D" targetname: "DryTest::Step::B" }

}
@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders May 17, 2017

Member

So, it doesn't explode on any of the supported modes. @oschwald have your requested changes been addressed?

Member

oalders commented May 17, 2017

So, it doesn't explode on any of the supported modes. @oschwald have your requested changes been addressed?

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders May 26, 2017

Member

@2shortplanks does this PR obsolete Stepford::Grapher?

Member

oalders commented May 26, 2017

@2shortplanks does this PR obsolete Stepford::Grapher?

@2shortplanks

This comment has been minimized.

Show comment
Hide comment
@2shortplanks

2shortplanks Jun 5, 2017

Member

Hello,

I've just come across this pull request and I was wondering how it compares to https://metacpan.org/release/Stepford-Grapher

In your opinion should this
a) Be something that supersedes Stepford::Grapher in core Stepford
b) Be something that should be merged into Stepford::Grapher
c) Something else

Thanks
Mark

Member

2shortplanks commented Jun 5, 2017

Hello,

I've just come across this pull request and I was wondering how it compares to https://metacpan.org/release/Stepford-Grapher

In your opinion should this
a) Be something that supersedes Stepford::Grapher in core Stepford
b) Be something that should be merged into Stepford::Grapher
c) Something else

Thanks
Mark

Outdated

@oschwald

This comment has been minimized.

Show comment
Hide comment
@oschwald

oschwald Jun 12, 2017

Member

What needs to be done to get this merged?

Member

oschwald commented Jun 12, 2017

What needs to be done to get this merged?

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Jun 12, 2017

Member

I think we just wanted to tackle whether this truly is a dry run or whether it's actually just dumping a dependency graph. The question of Stepford-Grapher isn't a blocker to merging this.

Member

oalders commented Jun 12, 2017

I think we just wanted to tackle whether this truly is a dry run or whether it's actually just dumping a dependency graph. The question of Stepford-Grapher isn't a blocker to merging this.

@jjatria

This comment has been minimized.

Show comment
Hide comment
@jjatria

jjatria Jun 12, 2017

Contributor

What is the criteria to tell between those two? What would make this truly a dry run, as opposed to dumping the dependency graph?

The way I initially understood the issue at least was that what was wanted was a printout of the steps that would be completed, without actually completing any of them. Since that information is stored in the dependency graph, that's what this patch uses for output (although they are printed from end to beginning).

If I misunderstood the issue, I'd be happy to correct it.

Contributor

jjatria commented Jun 12, 2017

What is the criteria to tell between those two? What would make this truly a dry run, as opposed to dumping the dependency graph?

The way I initially understood the issue at least was that what was wanted was a printout of the steps that would be completed, without actually completing any of them. Since that information is stored in the dependency graph, that's what this patch uses for output (although they are printed from end to beginning).

If I misunderstood the issue, I'd be happy to correct it.

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Jun 16, 2017

Member

Since discussion on this has stalled. I'm inclined to go ahead an merge it. Any objections?

Member

oalders commented Jun 16, 2017

Since discussion on this has stalled. I'm inclined to go ahead an merge it. Any objections?

@oschwald

This comment has been minimized.

Show comment
Hide comment
@oschwald

oschwald Jun 16, 2017

Member

Sounds good to me.

Member

oschwald commented Jun 16, 2017

Sounds good to me.

@oalders oalders merged commit c77310b into maxmind:master Jun 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Jun 16, 2017

Member

Thanks so much @jjatria. It was a long road, but eventually we got there. ;)

Member

oalders commented Jun 16, 2017

Thanks so much @jjatria. It was a long road, but eventually we got there. ;)

@jjatria jjatria deleted the jjatria:dry-run branch Jun 16, 2017

@jjatria

This comment has been minimized.

Show comment
Hide comment
@jjatria

jjatria Jun 16, 2017

Contributor

Totally worth it. Glad to be able to help! 🎉

Contributor

jjatria commented Jun 16, 2017

Totally worth it. Glad to be able to help! 🎉

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