Implements skip stage #2

rafaels88 · 2018-06-14T17:10:27Z

It introduces a new stage skip and solve the issue #1 .

Usage example:

 defmodule CreateUserPipeline do
   use Opus.Pipeline

   skip if: :user_exists?
   step :persist_user

   def user_exists?(_), do: true
 end

 CreateUserPipeline.call(%{}) # {:ok, :skipped}

zorbash · 2018-06-14T17:25:33Z

opus_opus_graph.dot

@@ -0,0 +1,68 @@
+digraph G {


Was this meant to be committed?

Actually I didn't know if it was meant to be commited since it was auto generated. I'll remove it!

zorbash · 2018-06-14T17:28:27Z

test/opus/pipeline/stage/skip_test.exs

+defmodule Opus.Pipeline.Stage.SkipTest do
+  use ExUnit.Case
+
+  describe "when the stage returns false and there's no next stage" do


We should add an example about what happens if there are multiple skips.

defmodule SomePipeline do skip if: :skip? skip if: :please_skip? skip if: :should_it_skip_maybe? step :some_step end

Sure! I'll add it

Added three more cases about multi skips, take a look

coveralls · 2018-06-14T18:23:44Z

Pull Request Test Coverage Report for Build 60

5 of 5 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 53:	0.0%
Covered Lines:	151
Relevant Lines:	151

💛 - Coveralls

zorbash · 2018-06-14T22:58:51Z

lib/opus/pipeline/stage/skip.ex

+
+  def run({module, type, [if: func], opts} = stage, input) do
+    case Stage.maybe_run({module, type, nil, opts |> put_in([:if], func)}, input) do
+      ret when ret == :stage_skipped ->


def run({module, type, [if: func], opts}, input) do case Stage.maybe_run({module, type, nil, opts |> put_in([:if], func)}, input) do :stage_skipped -> {:halt, :skipped} _ -> {:cont, input} end end

The guard is not needed here

When the conditional returns false (and we don't skip) you can return {:cont, input} so that the pipeline continues.

Totally, thanks for noticing! Just changed!

zorbash · 2018-06-14T23:00:24Z

lib/opus/pipeline/stage.ex

+  def maybe_run({module, :skip, name, %{if: {_m, _f, _a} = condition}}, input) do
+    case Safe.apply(condition) do
+      true ->
+        skip_stage({module, name}, input)


This is tricky because in this case the whole pipeline is skipped, not just a single stage. I think we should use a different instrumentation event, something like pipeline_skipped.

Ouch, really well noticed! Thanks again! Changed it, hope it's better now

zorbash · 2018-06-15T10:37:35Z

test/opus/pipeline/stage/skip_test.exs

+    defmodule SingleSkipFalsePipeline do
+      use Opus.Pipeline
+
+      skip(if: :should_skip?)


Please mind to update the .formatter.exs file.
https://github.com/Zorbash/opus/blob/master/.formatter.exs#L11

Ups, changing!

zorbash · 2018-06-15T10:40:58Z

test/opus/pipeline/stage/skip_test.exs

+    defmodule SkipFalsePipeline do
+      use Opus.Pipeline
+
+      skip(if: :should_skip?)


What happens if the user tries the following?

defmodule SomeMod do skip # without an :if option end

(CompileError) test/opus/pipeline/stage/skip_test.exs:127: undefined function skip/0

Make sense, wdyt?

And, when a param other then "if" is given: eg: skip another: :value

(FunctionClauseError) no function clause matching in Opus.Pipeline.Stage.Skip.run/2

zorbash · 2018-06-15T10:42:55Z

README.md

+
+This stage receives a `:if` option only, on which is expected to return a
+boolean value. If `true`, then the pipeline halts and Opus returns
+`{:ok, :skipped}`. If `false`, then the next stage is called with no side effect.


From the code I can see that if true we skip, otherwise (with any other value) we don't.

Should I just change the docs to something like If anything else, then the next stage is called with no side effect. or do you think that if no boolean is returned, then Opus should raise an error?

Just changed the description to: "If false or any other value is returned (including non-boolean), then the next stage is called with no side effect."

Also implemented a new spec for this case.

Let me know if that makes sense!

zorbash · 2018-06-15T10:43:50Z

lib/opus/pipeline/stage.ex

@@ -24,6 +24,22 @@ defmodule Opus.Pipeline.Stage do
      when is_atom(fun),
      do: maybe_run({module, type, name, %{opts | if: {module, fun, [input]}}}, input)

+  def maybe_run({module, :skip, name, %{if: {_m, _f, _a} = condition}}, input) do


There's some implied behaviour here I think. When _f is nil which means that no :if option is provided then we don't skip the pipeline, right? In that case it might be worth it raising a compile-time error since the skip definition will essentially be invalid.

Just mentioned about it above, but just to be clear, those are some tests I've tried:

No param

defmodule SkipNoParamPipeline do use Opus.Pipeline skip end

(CompileError) test/opus/pipeline/stage/skip_test.exs:127: undefined function skip/0

Non-keyword param

defmodule SkipAnotherParamPipeline do use Opus.Pipeline skip :another end

** (FunctionClauseError) no function clause matching in Opus.Pipeline.Stage.Skip.run/2 The following arguments were given to Opus.Pipeline.Stage.Skip.run/2: # 1 {Opus.Pipeline.Stage.SkipTest.SkipAnotherParamPipeline, :skip, :another, %{stage_id: 24357}}

Keyword param without if

defmodule SkipAnotherValuePipeline do use Opus.Pipeline skip another: :value end

** (FunctionClauseError) no function clause matching in Opus.Pipeline.Stage.Skip.run/2 The following arguments were given to Opus.Pipeline.Stage.Skip.run/2: # 1 {Opus.Pipeline.Stage.SkipTest.SkipAnotherValuePipeline, :skip, [another: :value], %{stage_id: 24772}}

Another keyword param and if

defmodule SkipAnotherValueAndIfPipeline do use Opus.Pipeline skip if: :cond, another: :value end

** (FunctionClauseError) no function clause matching in Opus.Pipeline.Stage.Skip.run/2 The following arguments were given to Opus.Pipeline.Stage.Skip.run/2: # 1 {Opus.Pipeline.Stage.SkipTest.SkipAnotherValueAndIfPipeline, :skip, [if: :cond, another: :value], %{stage_id: 28804}}

But, feel free to recommend any refactoring or implementation about this situation!

To conclude this discussion, I think it's fine to merge it as is. Then I'll make an attempt to make the error message a bit more user-friendly.

rafaels88 · 2018-07-14T08:28:11Z

Btw, sorry for the delay =/

zorbash · 2018-07-17T08:35:59Z

README.md

@@ -42,6 +42,7 @@ end
 defmodule ArithmeticPipeline do
  use Opus.Pipeline

+  skip                    if: greater_than_fifty?


greater_than_fifty? should be an Atom.

zorbash · 2018-07-17T08:36:48Z

README.md

@@ -112,6 +114,13 @@ This stage is to link with another Opus.Pipeline module. It calls
 `call/1` for the provided module. If the module is not an
 `Opus.Pipeline` it is ignored.

+### Skip
+
+This stage receives a `:if` option only, on which is expected to return a


I'd rephrase to This stage expects or This stage accepts.

zorbash · 2018-07-17T08:50:16Z

@rafaels88 Please make changes for

#2 (comment)
#2 (comment)

and it's good to merge :-)

The `:skip` stage makes the Pipeline halts when the given conditional returns `true`.

rafaels88 · 2018-07-17T16:51:18Z

@zorbash Changed! Thanks for the review!

zorbash · 2018-07-17T16:59:29Z

@rafaels88 Thanks for contributing, I'll update the changelog and releases it as 0.6.0.

zorbash reviewed Jun 14, 2018

View reviewed changes

rafaels88 force-pushed the implement-skip-stage branch 2 times, most recently from 6fe99f3 to 7e3b93a Compare June 14, 2018 20:40

zorbash reviewed Jun 14, 2018

View reviewed changes

rafaels88 force-pushed the implement-skip-stage branch from 7e3b93a to 5dbde21 Compare June 15, 2018 10:15

zorbash reviewed Jun 19, 2018

View reviewed changes

rafaels88 force-pushed the implement-skip-stage branch 2 times, most recently from ab407e1 to f2d4d97 Compare July 14, 2018 08:27

zorbash reviewed Jul 17, 2018

View reviewed changes

Implements skip stage

6122547

The `:skip` stage makes the Pipeline halts when the given conditional returns `true`.

rafaels88 force-pushed the implement-skip-stage branch from f2d4d97 to 6122547 Compare July 17, 2018 16:50

zorbash merged commit a895928 into zorbash:master Jul 17, 2018

zorbash added the enhancement New feature or request label Jul 17, 2018

rafaels88 deleted the implement-skip-stage branch July 18, 2018 16:44

zorbash mentioned this pull request Jul 19, 2018

Halting the pipeline without raising an error #1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements skip stage #2

Implements skip stage #2

rafaels88 commented Jun 14, 2018

zorbash Jun 14, 2018

rafaels88 Jun 14, 2018

rafaels88 Jun 14, 2018

zorbash Jun 14, 2018

rafaels88 Jun 14, 2018

rafaels88 Jun 14, 2018

coveralls commented Jun 14, 2018 •

edited

Loading

zorbash Jun 14, 2018

rafaels88 Jun 15, 2018

zorbash Jun 14, 2018

rafaels88 Jun 15, 2018

zorbash Jun 15, 2018

rafaels88 Jul 14, 2018

zorbash Jun 15, 2018

rafaels88 Jul 14, 2018

rafaels88 Jul 14, 2018 •

edited

Loading

zorbash Jun 15, 2018

rafaels88 Jul 14, 2018 •

edited

Loading

rafaels88 Jul 14, 2018

zorbash Jun 15, 2018

rafaels88 Jul 14, 2018

rafaels88 Jul 14, 2018

zorbash Jul 17, 2018

zorbash Jul 17, 2018

rafaels88 commented Jul 14, 2018

zorbash Jul 17, 2018

zorbash Jul 17, 2018

zorbash commented Jul 17, 2018

rafaels88 commented Jul 17, 2018

zorbash commented Jul 17, 2018

Implements skip stage #2

Implements skip stage #2

Conversation

rafaels88 commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 14, 2018 • edited Loading

Pull Request Test Coverage Report for Build 60

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaels88 Jul 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaels88 Jul 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaels88 commented Jul 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zorbash commented Jul 17, 2018

rafaels88 commented Jul 17, 2018

zorbash commented Jul 17, 2018

coveralls commented Jun 14, 2018 •

edited

Loading

rafaels88 Jul 14, 2018 •

edited

Loading

rafaels88 Jul 14, 2018 •

edited

Loading