-
Notifications
You must be signed in to change notification settings - Fork 0
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
Issue/52 ruby rails upgrade #53
Conversation
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.
Let's discuss!
s.add_dependency 'jquery-datatables-rails' | ||
s.add_dependency 'jquery-rails' | ||
s.add_dependency 'lodash-rails' | ||
s.add_dependency 'modulejs-rails' |
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.
Does Rubocop not complain about not having a targeted version? 🤔
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.
It doesn't for me, and it seems to be a lot of disagreement on this one as well (https://www.reddit.com/r/rails/comments/sujdqk/best_practice_should_i_specify_versions_in_gem/). Personally I prefer to leave the versions out because gems have great backwards compatibility (most of the time) and Gemfile.lock
has all the versions set anyway. If the app is a bit older and using old ruby / rails versions it makes sense to have them fixed for compatibility issues. This was the case with Qonsole, because of the rails constraint. If the app is using newer ruby / rails versions I think it makes future upgrades easier as you won't have to worry about upgrading gems manually from the gemspec, instead you would just nuke Gemfile.lock
and regenerate the file or run bundle update ...
. Lack of versions in the gemspec also helps with fixing dependabot alerts by making the process easier for the dependabot bot as he will bump versions in Gemfile.lock
and create PRs automatically
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.
LGTM! Nice work @bogdanadrianmarc! 🎉
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.
LGTM. Well done
This PR addresses issue #52 by upgrading ruby and rails for Qonsole to latest, as well as moving the
codemirror
package intovendor/assets