Skip to content
This repository was archived by the owner on Nov 17, 2020. It is now read-only.

write seeds for comments and dependencies - #20

Open
Polo2 wants to merge 2 commits into
masterfrom
create-seeds
Open

write seeds for comments and dependencies#20
Polo2 wants to merge 2 commits into
masterfrom
create-seeds

Conversation

@Polo2

@Polo2 Polo2 commented Sep 27, 2018

Copy link
Copy Markdown
Contributor

Some seeds to start using our toolbox.

@Polo2
Polo2 requested review from a user, michaelbaudino and thibaultdalban September 27, 2018 15:18
@michaelbaudino
michaelbaudino temporarily deployed to toolbox-staging-pr-20 September 27, 2018 15:18 Inactive

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not in favor to add those to db/seeds.rb which I would keep for "real"
data that is essential for running the application.

But if we find them useful, I'm not against adding this as a script in bin
directory, as a rake task, or maybe another way?

Comment thread db/seeds.rb Outdated
@@ -0,0 +1,18 @@
Dependency.destroy_all
Comment.destroy_all

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Whether it's "real" seeds, or fake data we would find useful in a development
workflow, I would not do this, we should never destroy all records for a model
in seeds IMO.

Comment thread db/seeds.rb Outdated
Dependency.destroy_all
Comment.destroy_all

puts "create 2 dependencies: d1 and d2"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would remove this. If we want more info about seeds, we can just read this
file, and when we execute the db:seed task, rails logs are printed on
standard output if I recall correctly.

Comment thread db/seeds.rb Outdated
Comment.destroy_all

puts "create 2 dependencies: d1 and d2"
d1 = Dependency.create(name: "Gem n°1")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd recommend Dependency#find_or_create_by!, so that it's created only once,
and fail directly when the creation is not possible.

Comment thread db/seeds.rb Outdated

puts "create 2 published comments; c1 and c2, related to dependency d2"
c1 = Comment.create(body: "published comment number 1", published: true, dependency: d2)
c2 = Comment.create(body: "published comment number 2", published: true, dependency: d2)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Local variables c0, c1, c2 are dead code (unused) I think.

Comment thread db/seeds.rb Outdated
puts "#{Dependency.count} created"
puts "#{Comment.count} created"

puts "Happy Growing !" No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As said, I'd remove all #puts calls. But we miss a final \n at end of file
here.

@ghost

ghost commented Sep 28, 2018

Copy link
Copy Markdown

Another remark: we must keep in mind that if we add this, we'll have to maintain it.

@Polo2
Polo2 temporarily deployed to toolbox-staging-pr-20 September 28, 2018 10:25 Inactive
@Polo2
Polo2 requested a review from a user September 28, 2018 10:43
@Polo2

Polo2 commented Sep 28, 2018

Copy link
Copy Markdown
Contributor Author

I moved this behavior on a rake task db:dev_seeds, thus:

  • it's not about what we expect about seeds.rb should no or not
  • it's only a dev script to clean database and keep a few starting datas

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants