Closed tkuchiki closed 6 years ago
Thanks for the PR! This project lacks automated tests, which makes reviewing and approving PRs somewhat difficult. Please make sure that your contribution has not broken backwards compatibility or introduced any risky changes.
Generated by :no_entry_sign: Danger
Hi, this seems like a useful feature. Thanks for the PR! I appreciate the tweaks you made for coding style and adding a good CHANGELOG entry.
Before merging this, I would like to suggest some minor changes to make the code a bit more clear.
:assets_manifest
value to an array if it is not one already. This allows the user to supply a single value or an array of values. It also means that the default behavior can use the same mechanism, and not be a special case.:assets_manifest
in load:defaults
instead of relying on a special nil
conditional for the default behavior. This should simplify the logic.Perhaps the default value could be something like this:
# Default set in load:defaults
set :assets_manifest, -> {
%w[.sprockets-manifest* manifest*.*].map do |pattern|
release_path.join("public", fetch(:assets_prefix), pattern)
end
}
Also, could you rebase and fix the conflict? Thanks!
@mattbrictson Thank you for your feedback! I think you’re right. I will fix it.
@mattbrictson Fixed.
How about using Array() to coerce the user-supplied :assets_manifest value to an array if it is not one already. This allows the user to supply a single value or an array of values. It also means that the default behavior can use the same mechanism, and not be a special case.
assets_manifest
rename to assets_manifests
to clarify that it is a Array.
Would this be okay?
Very need this fix
Actually there is manifest file in /public/pack directory, but capistrano/rails watch only /public/assets Need to deal with it))
[10 minutes later...]
Fixed by set :assets_prefix, 'packs'
This looks good! I'll test it this week with an actual deploy and merge it if it all works.
All checks out 👍
Congrats on your first contribution to the project!
I plan to push a new version of the gem this weekend.
Thank you for reviewing this PR. I'm looking forward to release!
Added option ':assets_manifest' to support custom manifest file path.
Without this option and set
config.assets.manifest
inconfig/application.rb
, the following error message appears.config.assets.manifest
is used in https://github.com/rails/sprockets-rails.