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

some code, typos and consistency fixes #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

peterberkenbosch
Copy link

While going through this awesome step by step post, I ran into some issues. This commit is an attempt to give back by updating this post with a couple of fixes.

Copy link
Owner

@radar radar left a comment

Choose a reason for hiding this comment

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

Thank you so much for your diligent feedback :) I've got a few comments and then I think this will be good to merge.

@@ -217,12 +217,12 @@ Then the migration has been successfully applied.

In order to get data into and out of database tables with ROM, we need to create something called a _repository_. A repository is a class that is used to define a clear API between your database and your application.

To create one of these, we'll create a new file inside a new directory structure at `lib/bix/repos/user_repo.rb`:
To create one of these, we'll create a new file inside a new directory structure at `lib/bix/repos/user.rb`:
Copy link
Owner

Choose a reason for hiding this comment

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

I want to leave this as user_repo because later on when we import it with dry-auto_inject it'll make it available as a variable called user_repo, not user.

Copy link
Author

Choose a reason for hiding this comment

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

Sure thing!, in the code samples it's mostly Bix::Repos::User though, also in part 2 (haven't been up to part 3 ;). Will have to update the code samples to be Bix::Repos::UserRepo as well then.

@@ -234,12 +234,12 @@ To use this class (and others that we will create later on), we'll need to creat
```ruby
Bix::Application.boot(:persistence) do |app|
start do
register('container', ROM.container(:sql, app['db.connection']))
register('container', ROM.container(:sql, app['db.config'].gateways[:default].connection))
Copy link
Owner

Choose a reason for hiding this comment

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

You're right that this line needs changing, but I just figured out you can do this:

register('container', ROM.container(app['db.config']))

Copy link
Author

Choose a reason for hiding this comment

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

This was my first attempt as well, it did not work for me. The db.config returns a ROM::Configuration instance, where the container expects a database connection.

Copy link
Owner

Choose a reason for hiding this comment

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

Yup, there's a bit more that needs to change. I've revised Part 1 just this morning and the relevant changes for this part are: 57aae3a#diff-0de0c2c310f3cf59a6ea6fdc92b1af3aR368

Copy link
Author

Choose a reason for hiding this comment

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

awesome! I finally got to the point where I actually understand the need for user_repo.rb and create_user.rb. The dependency injection makes it so much more readable.

Copy link
Owner

Choose a reason for hiding this comment

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

It's so great! :D

Copy link
Author

Choose a reason for hiding this comment

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

figured out we can use aliases! Love it even more.. Import[user_repo: 'repos.user']

Copy link
Owner

Choose a reason for hiding this comment

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

Oh great, I didn't know about those aliases.

@@ -254,7 +254,7 @@ require 'irb'
IRB.start
```

This file will load our application's `config/application.rb` file. When this file is loaded, all the files in `lib` will be required. This includes our new `lib/bix/repos/user_repo.rb` file.
This file will load our application's `config/application.rb` file. When this file is loaded, all the files in `lib` will be required. This includes our new `lib/bix/repos/user.rb` file.
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep this as user_repo.rb

@@ -363,7 +363,7 @@ However, we will need to register relations with our application's database cont
```ruby
Bix::Application.boot(:persistence) do |app|
start do
container = ROM.container(:sql, app['db.connection']) do |config|
container = ROM.container(:sql, app['db.config'].gateways[:default].connection) do |config|
Copy link
Owner

Choose a reason for hiding this comment

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

This should be:

ROM.container(app['db.config'])

@@ -381,12 +381,12 @@ Let's run `bin/console` again and try working with our repository again:
NoMethodError (undefined method `all' for #<Bix::Repos::User struct_namespace=ROM::Struct auto_struct=true>)
```

Oops! Repositores are intentionally bare-bones in ROM; they do not come with very many methods at all. Let's exit the console and then we'll define some methods on our repository. While we're here, we'll add a method for finding all the users, and one for creating users. Let's open `lib/bix/repos/user_repo.rb` and add these methods:
Oops! Repositories are intentionally bare-bones in ROM; they do not come with very many methods at all. Let's exit the console and then we'll define some methods on our repository. While we're here, we'll add a method for finding all the users, and one for creating users. Let's open `lib/bix/repos/user.rb` and add these methods:
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep this as user_repo.rb.


```ruby
module Bix
module Repos
class UserRepo < ROM::Repository[:users]
class User < ROM::Repository[:users]
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep this as UserRepo

@@ -427,7 +427,7 @@ Hooray! We have now been able to add a record and retrieve it. We have now set u
* `system/boot/db.rb` - Specifies how our application connects to a database
* `system/boot/persistence.rb` - Defines a ROM container that defines how the ROM pieces of our application connect to and interact with our database
* `lib/bix/relations/users.rb` - Defines a class that can contain query logic for our `users` table
* `lib/bix/repositories/user.rb` - A class that contains methods for interacting with our relation, allowing us to create + retrieve data from the databse.
* `lib/bix/repos/user.rb` - A class that contains methods for interacting with our relation, allowing us to create + retrieve data from the database.
Copy link
Owner

Choose a reason for hiding this comment

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

Please switch this to user_repo.rb.

@@ -449,12 +449,12 @@ end

Ignoring [the falsehoods programmers believe about names](https://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/), this method will combine a user's `first_name` and `last_name` attributes.

To use this class though, we need to configure the repository further over in `lib/bix/repos/user_repo.rb`:
To use this class though, we need to configure the repository further over in `lib/bix/repos/user.rb`:
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep this as user_repo.rb.


```ruby
module Bix
module Repos
class UserRepo < ROM::Repository[:users]
class User < ROM::Repository[:users]
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep this as UserRepo.

@@ -519,12 +519,12 @@ module Bix
end
```

This `Import` constant will allow us to import (or _inject_) anything registered with our application into other parts. Let's see this in action now by adding this line to `lib/repos/user_repo.rb`:
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep this as user_repo.rb

@radar
Copy link
Owner

radar commented Feb 20, 2020

Please pull down latest master and resolve conflicts.

While going through this awesome step by step post, I ran into some issues. This commit is an attempt to give back by updating this post with a couple of fixes.
@peterberkenbosch
Copy link
Author

will do second pass in my am. Some UserRepo and user_repo.rb are needed in this PR still.

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.

None yet

2 participants