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

Tutorial for Append Only Database/Log #2

Merged
merged 16 commits into from
Sep 27, 2018
Merged

Tutorial for Append Only Database/Log #2

merged 16 commits into from
Sep 27, 2018

Conversation

Danwhy
Copy link
Member

@Danwhy Danwhy commented Sep 12, 2018

Adds basic guide for creating a schema and a behaviour to facilitate an append only database, with code and tests.

There are still some improvements/advancements that could be made to the guide (see open questions in #1), but this should be a good starting point

@Danwhy Danwhy self-assigned this Sep 12, 2018
@nelsonic
Copy link
Member

nelsonic commented Sep 17, 2018

@Danwhy I'm going to take a look at this tonight.
please remove "[WiP]" if you feel it's "mergeable". thanks! 👍

Following along ...

Create SQL user:
image

Install deps
image

Create Database:
image

Migrate:
image

So far, so fail:
image

👍

@Danwhy Danwhy changed the title Tutorial for Append Only Database/Log [WIP] Tutorial for Append Only Database/Log Sep 19, 2018
@nelsonic
Copy link
Member

test add item to database (Append.AddressTest)
     test/append/address_test.exs:5
     ** (UndefinedFunctionError) function Repo.insert/1 is undefined (module Repo is not available)

image
😞

README.md Outdated

``` elixir
defmodule Append.AppendOnlyLog do
...
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have alias Append.Repo on this line. Sorry @nelsonic! I'll add it in

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. don't apologize. you've done a great job with this guide (so far!) 🎉
(thanks! hope you're feeling much better!)

@nelsonic
Copy link
Member

nelsonic commented Sep 20, 2018

Have to add:

  alias Append.Repo

To the top of lib/append/append_only_log.ex
as per: https://github.com/dwyl/phoenix-ecto-append-only-log-example/pull/2/files#diff-ba1c4c1b3f67bc03f7691e3253abce7dR2

then tests will pass:
image

adding to code/instructions in README.md 😉


def update(%__MODULE__{} = item, attrs) do
item
|> Map.put(:id, nil)
Copy link
Member

@nelsonic nelsonic Sep 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unclear why we need to remove the :id
is it not auto-incremented by Postgres when the record is inserted?

@codecov
Copy link

codecov bot commented Sep 21, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@165818e). Click here to learn what that means.
The diff coverage is 56.75%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master       #2   +/-   ##
=========================================
  Coverage          ?   56.75%           
=========================================
  Files             ?       11           
  Lines             ?       37           
  Branches          ?        0           
=========================================
  Hits              ?       21           
  Misses            ?       16           
  Partials          ?        0
Impacted Files Coverage Δ
test/support/channel_case.ex 0% <0%> (ø)
lib/append_web.ex 0% <0%> (ø)
lib/append_web/views/error_helpers.ex 0% <0%> (ø)
lib/append/address.ex 100% <100%> (ø)
test/support/conn_case.ex 100% <100%> (ø)
lib/append_web/controllers/page_controller.ex 100% <100%> (ø)
lib/append_web/views/error_view.ex 100% <100%> (ø)
lib/append_web/endpoint.ex 50% <50%> (ø)
test/support/data_case.ex 57.14% <57.14%> (ø)
lib/append_web/router.ex 66.66% <66.66%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 165818e...72394c2. Read the comment docs.

postcode: "NW SCA",
})

history = Address.get_history(updated_item)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be an {:ok, history} = Address.get_history(updated_item) tuple pattern match?
or does Repo.all(query) always return a List (even if it's empty) ?

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Danwhy this is an excellent start.
I was able to follow along on my localhost without any issues (other than those described/fixed above):
image

Happy to merge this so others can benefit from it.
And we can iterate on it further when we have time.

@Cleop do you prefer to try follow the steps in PR/branch form or can I merge to master ?

Thanks again! ❤️ ✅

@nelsonic nelsonic assigned Cleop and unassigned nelsonic Sep 22, 2018
Copy link
Member

@Cleop Cleop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to merge @nelsonic, I'll create a separate PR from master for a couple of tweaks. Nice one @Danwhy 🎉

@nelsonic nelsonic mentioned this pull request Sep 26, 2018
11 tasks
@nelsonic
Copy link
Member

@Cleop thanks. 👍

@nelsonic
Copy link
Member

Note: we still need to add the answers to the "intro questions": #1 (comment)

I'm going to add those to a separate PR shortly.
Merging this for now. 👍

@nelsonic nelsonic merged commit d37c776 into master Sep 27, 2018
@nelsonic nelsonic deleted the tutorial branch September 27, 2018 14:20
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