-
Notifications
You must be signed in to change notification settings - Fork 271
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
Introduce Postgrex.Stage #312
Conversation
lib/postgrex/stage.ex
Outdated
@@ -0,0 +1,93 @@ | |||
defmodule Postgrex.Stage do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modules should have a @moduledoc tag.
end | ||
|
||
defp copy_data(conn, data, %Postgrex.Copy{ref: ref} = copy, opts) do | ||
try do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer using an implicit try
rather than explicit try
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry @ebert[bot], I got your back. :P
lib/postgrex/protocol.ex
Outdated
@@ -1276,10 +1280,10 @@ defmodule Postgrex.Protocol do | |||
sync_recv(s, status, result, buffer) | |||
end | |||
|
|||
defp suspend(s, status, query, cursor, rows, buffer) do | |||
defp suspend(s, status, query, _cursor, rows, buffer) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function takes too many parameters (arity is 6, max is 5).
test/stage_test.exs
Outdated
test "produce COPY TO STDOUT", context do | ||
query = "COPY (VALUES (1, 2), (3, 4)) TO STDOUT" | ||
parent = self() | ||
opts = [stream_mapper: fn(_, %{rows: rows}) -> send(parent, rows) ; rows end] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use ; to separate statements and expressions
test/stage_test.exs
Outdated
test "produce query in chunks", context do | ||
query = prepare("", "SELECT * FROM generate_series(1, 3)") | ||
parent = self() | ||
opts = [stream_mapper: fn(_, %{rows: rows}) -> send(parent, rows) ; rows end] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use ; to separate statements and expressions
test/stage_test.exs
Outdated
defmodule StageTest do | ||
use ExUnit.Case, async: true | ||
import Postgrex.TestHelper | ||
alias Postgrex.{CopyConsumer, Producer} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the time you are using the multiple single line alias/require/import/use directives but here you are using the multi-alias/require/import/use syntax
Ebert has finished reviewing this Pull Request and has found:
But beware that this branch is 1 commit behind the You can see more details about this review at https://ebertapp.io/github/elixir-ecto/postgrex/pulls/312. |
lib/postgrex/producer.ex
Outdated
|
||
## Examples | ||
|
||
{:ok, pid} = Postgrex.Producer.start_link(pool, "SELECT * FROM posts, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing closing quotes.
|
||
{:ok, pid} = Postgrex.Producer.start_link(pool, "SELECT * FROM posts, []) | ||
[{pid, [cancel: :transient]}] | ||
|> GenStage.stream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this example, let's make an imaginary consumer subscribe to the producer. That's because this use case is better done with streams.
❤️ 💚 💙 💛 💜 |
|
||
## Examples | ||
|
||
{:ok, pid} = Postgrex.Producer.start_link(pool, "SELECT * FROM posts, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the closing "
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look, we aren't going to go ahead with this approach. Please see #321.
This is WIP. Includes a common real world example of a
:consumer
stage.