-
Notifications
You must be signed in to change notification settings - Fork 124
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
Gemfile - update engine cart stanza for v1.1.0 #1010
Conversation
@cbeer - add rubocop checks on engine_cart so it's stanza passes rubocop checks.
This motived this issue on engine_cart at cbeer/engine_cart#80 |
Gemfile
Outdated
# Override the coffee-rails version in engine_cart for rails 4.2 | ||
case ENV['RAILS_VERSION'] | ||
when /^4.2/ | ||
gem 'coffee-rails', '~> 4.2.0' |
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.
This seems like it conflicts with line 44. Can you say more about why we need both?
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.
The line-44 code is maintained by engine-cart, it should not be modified by anything but engine cart and we should assume that engine-cart will not respect any changes in the block it maintains. To override anything in that block, it must be declared separately. So, in this PR, to preserve the intention of a prior commit, this block was added in the last commit.
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.
A separate issue is whether this override is actually needed at all any more, so I did not intend to address that in this PR.
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.
@darrenleeweber but we don't support rails 4.2, and this block appears to specifically be for Rails 4.2.
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.
The line-44 code is maintained by engine-cart, it should not be modified by anything but engine cart
I wouldn't go that far -- instead, I'd say: by modifying it, the onus is on the project maintainers to ensure it is maintained the way they expect.
I thought having duplicate gem declarations (particularly conflicting ones) makes bundler unhappy?
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.
Good question - I have not tested what happens when the rails version hits this code. If hyrax is no longer compatible with rails 4.x, then I should just remove the last commit and leave that stanza as engine-cart creates it (i.e. discard the override entirely).
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 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.
Removed the last commit to override anything on rails 4.2
fefd490
to
b7284b0
Compare
Gemfile
Outdated
# Override the coffee-rails version in engine_cart for rails 4.2 | ||
case ENV['RAILS_VERSION'] | ||
when /^4.2/ | ||
gem 'coffee-rails', '~> 4.2.0' |
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.
@darrenleeweber but we don't support rails 4.2, and this block appears to specifically be for Rails 4.2.
Related to cbeer/engine_cart#79 (does not fix it in engine_cart)
@projecthydra-labs/hyrax-code-reviewers
@mjgiarlo - I just discovered that this could undo a prior hack on the engine-cart stanza: