-
Notifications
You must be signed in to change notification settings - Fork 97
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
Refactor of gem files and rake tasks #157
base: master
Are you sure you want to change the base?
Conversation
ab4d5ac
to
64cb4ca
Compare
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.
Great work! 💯
I wonder if we can rename .document
to .rdoc_options
?
https://ruby.github.io/rdoc/RDoc/Options.html#class-RDoc::Options-label-Saved+Options
Rakefile
Outdated
rescue LoadError | ||
task :rcov do | ||
abort "RCov is not available. In order to run rcov, you must: sudo gem install spicycode-rcov" | ||
desc "Look for TODO and FIXME tags in the code" |
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'd probably drop this, easy with project-wide search.
But I'm not against keeping it if you prefer.
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 would delete it too.
Gemfile
Outdated
end | ||
gemspec | ||
|
||
gem "minitest" |
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.
Any reason for dropping the group :development do
?
I'd prefer keeping it, but maybe there is a good reason for dropping?
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.
In a gem, the Gemfile
file is always used for development, so it is not necessary to differentiate by groups and they can go to the Bundler default group (:default
) and thus the Gemfile
file is much simpler.
Gemfile
Outdated
end | ||
gemspec | ||
|
||
gem "minitest" |
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 think we can specify a minimum version for these gems, since they are dev-dependencies and not gem dependencies.
gem 'minitest', '~> 5.8'
(or similar)
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 Gemfile.lock
file is now versioned within git, so all developers will install, when they run the bundler install
command, the same version of the libraries that the project requires for its development.
https://bundler.io/v2.5/man/bundle-install.1.html#THE-GEMFILE-LOCK
On the other hand, the dependencies of the gem when it is installed in other projects are marked by what is indicated in the gemspec, although this gem does not have any runtime dependencies.
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.
Thanks, yes I know how all this works.
However, I think it's useful that the Gemfile
signals intent. For example `gem 'minitest', '~> 5.8' signals that we're not expecting minor updates to cause any trouble, so those are alright to update automatically, or without much thought, but major versions need to be more explicit.
https://stackoverflow.com/questions/9265213/should-i-specify-exact-versions-in-my-gemfile
https://www.reddit.com/r/rails/comments/sujdqk/best_practice_should_i_specify_versions_in_gem/
Can you put these back?
gem 'minitest', '~> 5.8'
gem 'rake', '~> 13.1'
gem 'simplecov', '~> 0.22', require: false
Other than this, I think this is ready to go! 💯
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, of course!
If I'm honest, I don't know what this file is for and who uses it. |
bfe69b5
to
a64ce60
Compare
I also refactored |
Gemfile
Outdated
end | ||
gemspec | ||
|
||
gem "minitest" |
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.
Thanks, yes I know how all this works.
However, I think it's useful that the Gemfile
signals intent. For example `gem 'minitest', '~> 5.8' signals that we're not expecting minor updates to cause any trouble, so those are alright to update automatically, or without much thought, but major versions need to be more explicit.
https://stackoverflow.com/questions/9265213/should-i-specify-exact-versions-in-my-gemfile
https://www.reddit.com/r/rails/comments/sujdqk/best_practice_should_i_specify_versions_in_gem/
Can you put these back?
gem 'minitest', '~> 5.8'
gem 'rake', '~> 13.1'
gem 'simplecov', '~> 0.22', require: false
Other than this, I think this is ready to go! 💯
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.
Nice! 👍🏻
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.
Great work! 💯
This PR updates the gem development environment, some rake tasks, and the gemspec. Needs to be rebased when my other PRs are merged!