Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

javierav
Copy link

@javierav javierav commented Mar 3, 2024

This PR updates the gem development environment, some rake tasks, and the gemspec. Needs to be rebased when my other PRs are merged!

@javierav javierav force-pushed the gem-files branch 2 times, most recently from ab4d5ac to 64cb4ca Compare March 3, 2024 12:33
Copy link
Collaborator

@sandstrom sandstrom left a 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"
Copy link
Collaborator

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.

Copy link
Author

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"
Copy link
Collaborator

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?

Copy link
Author

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.

VERSION Outdated Show resolved Hide resolved
lib/ipaddress/version.rb Outdated Show resolved Hide resolved
Gemfile Outdated
end
gemspec

gem "minitest"
Copy link
Collaborator

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)

Copy link
Author

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.

Copy link
Collaborator

@sandstrom sandstrom Mar 5, 2024

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! 💯

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course!

@javierav
Copy link
Author

javierav commented Mar 4, 2024

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

If I'm honest, I don't know what this file is for and who uses it.

@javierav javierav force-pushed the gem-files branch 2 times, most recently from bfe69b5 to a64ce60 Compare March 4, 2024 19:42
@javierav
Copy link
Author

javierav commented Mar 4, 2024

I also refactored Rake::RDocTask in Rakefile. Check it!

@javierav javierav requested a review from sandstrom March 4, 2024 19:54
Gemfile Outdated
end
gemspec

gem "minitest"
Copy link
Collaborator

@sandstrom sandstrom Mar 5, 2024

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! 💯

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 👍🏻

Copy link
Collaborator

@sandstrom sandstrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants