-
Notifications
You must be signed in to change notification settings - Fork 134
Commit
- Loading branch information
There are no files selected for viewing
6 comments
on commit d6492e6
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.
I resisted the temptation to implement this myself to force myself to think about the blueprints differently from fixtures. It has the potential to lead you back to fixture mindset I think.
Using the master as the base for a named blueprint’s undefined attributes is great idea. This makes it more macro-like rather than fixture.
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.
You have implemented a “default” feature too, great!
About the discussion of Machinist being more fixtures like, I disagree. The basic functionality remains untouched. The fact that you can create named blueprints doesn’t involve that you must use named blueprint. For most models, the basic idea of 1 model : 1 blueprint works awesomely, it’s fast, it’s easy to change, to read, to manage, it’s clean and it does everything for you, without those dumb yml files everywhere.
But this change, as I see it, it’s necessary. Necessary for testing large models, in which ‘fake’ data doesn’t work so well (user roles, for example). They were hard to test without a way to name blueprints, since you must repeat the attribute values for a specific tests too many times.
Now it will be the same as before, but with an option for complex models to use named blueprints.
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.
I was resistant to adding this feature precisely because it encourages fixture-like coding, and almost all the use cases people gave me were bad practice.
The “variations on a theme” use case (e.g. users with different roles) is a good one however. I think encouraging a “master” blueprint and then using the named blueprints to specify just the differences for each variation keeps things tidy.
I can still user plain old User.make if I don’t care about the role, and working out what differentiates a user in a particular role from a generic user is easy (as opposed to the same sort of thing done with fixtures.)
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.
There’s an error in the spec, @block_called will never be set to true, even if the block is run, since a local variable is set in the block. You could change it to:
```
before do
block_called = false
Person.blueprint do
name { “Fred” }
admin { block_called = true; false }
end
Person.blueprint(:admin) do
admin { true }
end
@person = Person.make(:admin)
@block_called = block_called
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.
changing
admin { block_called = true; false }
to
admin { @block_called = true; false }
should do it as well yeah?
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.
You’re right guys. Well spotted. I’ll fix it.
Is it possible to create inheritance in blueprints? This way, rather than a "helper" method I can continue to use the Blueprint DSL and simply create an Admin blueprint which inherrits from User.