-
Notifications
You must be signed in to change notification settings - Fork 21.8k
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
Add an after_bundle callback in Rails templates #16359
Conversation
template = %{ after_bundle { run 'echo ran after_bundle' } } | ||
template.instance_eval "def read; self; end" # Make the string respond to read | ||
|
||
generator([destination_root], template: path).expects(:open).with(path, 'Accept' => 'application/x-thor-template').returns(template) |
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.
Could you avoid using Mocha please ? We are trying to remove it from the test suite. Thanks for the patch so far, it looks great! 👍
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 can try, but I'm not sure how. I arrived at this code by following the style of the surrounding tests. I want to check that:
- `bundle gets run
- ...then binstubs get generated
- ...then the
after_bundle
callbacks get executed
Surely the test cannot invoke bundle
, since it will make them (1) nondeterministic and (2) dependant on internet connection. Also, (3) very slow. Calls to the first two need to get stubbed in some way.
If I'm not allowed to use Mocha, I'm not sure how to handle it. I can define singleton methods on generator
, but that will surely get messier and harder to understand.
There is also the business of supplying a template. Since a string containing code cannot be passed to generator
, there's this path
and template.instance_eval
business that I borrowed from a test in the same suite. If I'm to remove Mocha, we should also figure out how to do that.
Finally, if the other tests in the suite can benefit from the same approach. Maybe there are a few nice abstractions that can be extracted and all the tests can benefit from them. This feels like another, larger task. I'd love to do it also (in a separate PR possibly), but I'll definitely need some hints on how to do it :)
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.
@robin850 given that the surrounding tests use the same pattern I don't think we need to tackle this in the same PR. Basically if one does remove Mocha for one of the tests it should be trivial to remove it for the others. They are all doing mostly the same stuff.
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.
Yes. No bother with mocha. I'll deal with it.
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.
given that the surrounding tests use the same pattern I don't think we need to tackle this in the same PR
Ah fair point 👍
@skanev does |
@itsNikolay Yup, the |
@skanev I think the docs should mention that |
@senny I've added it to the guide. |
The template runs before the generation of binstubs – this does not allow to write one, that makes an initial commit to version control. It is solvable by adding an after_bundle callback.
I've rebased onto master, squashed the two commits into one and added a reference to the fixed issue in the changelog. |
As this is a breaking change (well not directly but the user needs to modify his templates) we should include it in the release notes and the upgrading guide. |
I've added it to the release notes and upgrade guide in c294e91. |
Add an after_bundle callback in Rails templates Conflicts: railties/CHANGELOG.md
Closes #16292.
The following Rails template (taken from the Rails guides) does not work well with binstubs:
Since it is executed before Bundler and Spring binstub generation, the generated stubs are not added to version control. Executing the templates later wouldn't work either – that way it won't be able to add gems.
Adding an
after_bundle
callback seems to solve the issue. That way, the version control commands can be postponed after the binstubs have been generated:This pull request implements that callback. The test is a mouthful – I'm open to suggestions for improvements 😄
/cc @senny