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

More Code Examples from the Programming Phoenix book #42

Merged
merged 34 commits into from
Mar 6, 2017
Merged

Conversation

nelsonic
Copy link
Member

@nelsonic nelsonic commented Feb 27, 2017

@iteles adding code examples for the Programming Phoenix book
(I've personally written and tested the examples and been opening issues along the way e.g #38 so other people can learn faster...) 👍

Fixes #35, #49 & #50

Please review/merge when you can thanks.

@nelsonic nelsonic self-assigned this Feb 27, 2017
@nelsonic nelsonic assigned iteles and unassigned nelsonic Feb 27, 2017
@nelsonic nelsonic requested a review from iteles February 27, 2017 18:56
@nelsonic nelsonic assigned samhstn and unassigned iteles Feb 28, 2017
Copy link
Member

@samhstn samhstn left a comment

Choose a reason for hiding this comment

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

Just a couple of comments, but looks good.

Happy to merge after those are addressed.

I have been thinking about the syntax a little and would like your thoughts on this:

  • We use Word Sigils throughout the book, but I feel that using just a normal List of strings instead would be more readable in general (perhaps I will find these to be nicer when I'm more used to the syntax).
  • The String.contains syntax is initally more readable, but I have found the =~ syntax to be much nicer once you are used to it

Lastly, since we are running tests in the project, perhaps we could get these hosted?


for category <- ~w(Action Drama Romance Comedy Sci-fi) do
Repo.insert!(%Category{name: category}) ||
Repo.insert!(%Category{name: category})
Copy link
Member

Choose a reason for hiding this comment

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

I think these should be different, I have:

  Repo.get_by(Category, name: category) ||
    Repo.insert!(%Category{name: category})

instead

Copy link
Member Author

Choose a reason for hiding this comment

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

@Shouston3 good shout. please feel free to edit the file in PR. 👍
else I will fix tomorrow morning. 😉

create table(:categories) do
add :name, :string, null: false

timestamps() # function call parenthesis not in book!
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

end

test "login with password missmatch", %{conn: conn} do
_ = insert_user(username: "me", password: "secrete")
Copy link
Member

Choose a reason for hiding this comment

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

"secrete" typo

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, worth fixing. thanks. 👍

# test "changeset with invalid attributes" do
# changeset = User.changeset(%User{}, @invalid_attrs)
# refute changeset.valid?
# end
Copy link
Member

@samhstn samhstn Mar 1, 2017

Choose a reason for hiding this comment

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

I also have these comments in mine 👍


def create(conn, %{"video" => video_params}, user) do
IO.inspect video_params
IO.inspect user
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps remove these IO.inspects

user
|> build_assoc(:videos)
|> Video.changeset(video_params)
IO.inspect changeset
Copy link
Member

Choose a reason for hiding this comment

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

IO.inspect

Copy link
Member Author

Choose a reason for hiding this comment

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

Will remove. in next commit. 👍


timestamps()
timestamps() # invocation parenthesis not in book
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

Ugh this has been confusing us with warnings

Copy link
Member Author

Choose a reason for hiding this comment

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

@des-des are you running the latest version of Phoenix?

Copy link
Member

Choose a reason for hiding this comment

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

@nelsonic yeah 1.2.1

@samhstn samhstn assigned nelsonic and unassigned samhstn Mar 1, 2017
@@ -0,0 +1,29 @@
let Player = {
Copy link
Member

Choose a reason for hiding this comment

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

JS -> Elm?!

Copy link
Member Author

Choose a reason for hiding this comment

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

@des-des this example is from "The Book" ...
I don't intend to keep the ES6 after I've finished writing out the code. 😉

Copy link
Member

Choose a reason for hiding this comment

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

hehe

@nelsonic
Copy link
Member Author

nelsonic commented Mar 5, 2017

Totes works!
image

@nelsonic
Copy link
Member Author

nelsonic commented Mar 6, 2017

@iteles I've addressed the review comments from @Shouston3 & @des-des
and added Travis-CI to confirm that the demo app is working.
Please take a look at the code when you have time and merge if ok.
thanks! 👍

@nelsonic nelsonic assigned iteles and unassigned nelsonic Mar 6, 2017
let Player = {
player: null,

let Player = { // JS works for small scripts like this
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Copy link
Member

Choose a reason for hiding this comment

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

Also loving this pragmatism 👍

Copy link
Member

@iteles iteles left a comment

Choose a reason for hiding this comment

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

This has been pretty fascinating to review. Nothing to add for now, thanks for putting it together!

@iteles iteles merged commit 59cb5d2 into master Mar 6, 2017
@iteles iteles deleted the code-examples branch March 6, 2017 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants