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.

Sign up for GitHub

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

Jump to bottom

Use Gem::Version to compare versions #1236

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Use Gem::Version to compare versions #1236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8d04d7c
Prevent warnings if a generator tries to run
louis-antonopoulos Oct 29, 2024
a424bd7
Simplify node_not_installed logic
louis-antonopoulos Oct 29, 2024
a9d6878
User Gem:::Version for Node version comparisons
louis-antonopoulos Oct 29, 2024
579a24d
Update test description
louis-antonopoulos Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions lib/install/web.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
def node_version
ENV["NODE_VERSION"] || `node --version`[/\d+\.\d+\.\d+/]
version = ENV["NODE_VERSION"] || `node --version`[/\d+\.\d+\.\d+/]

return if version.blank?

Gem::Version.new(version)
Comment on lines +4 to +6
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think these two lines are clearer as written or combined like this?

Gem::Version.new(version) unless version.blank?

Comment on lines +2 to +6
Copy link

Choose a reason for hiding this comment

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

Would the following work?

Suggested change
version = ENV["NODE_VERSION"] || `node --version`[/\d+\.\d+\.\d+/]
return if version.blank?
Gem::Version.new(version)
if version = ENV.fetch(ENV["NODE_VERSION"], `node --version`[/\d+\.\d+\.\d+/])
Gem::Version.new(version)
else
""
end

It solves a couple of issues.

  1. Deals with #[] returning something falsy but not nil
  2. Gets rid of the trailing conditional and makes the method more idiomatic
  3. Ensures the method always returns a string

end

def node_not_installed?
!node_version.present?
node_version.blank?
Copy link
Contributor Author

@louis-antonopoulos louis-antonopoulos Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I changed this to avoid the double negative of not_installed and
!(...).present?.

end

def node_version_unsupported?
node_version < "20.0.0"
minimum_node_version = Gem::Version.new("20.0.0")

node_version < minimum_node_version
end

def apply_template!
Expand Down
12 changes: 9 additions & 3 deletions lib/suspenders/generators.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,21 @@ def rspec_test_helper_present?
end

def node_version
ENV["NODE_VERSION"] || `node --version`[/\d+\.\d+\.\d+/]
version = ENV["NODE_VERSION"] || `node --version`[/\d+\.\d+\.\d+/]

return if version.blank?

Gem::Version.new(version)
end

def node_not_installed?
!node_version.present?
node_version.blank?
end

def node_version_unsupported?
node_version < Suspenders::MINIMUM_NODE_VERSION
minimum_node_version = Gem::Version.new(Suspenders::MINIMUM_NODE_VERSION)

node_version < minimum_node_version
end
end

Expand Down
22 changes: 22 additions & 0 deletions test/generators/suspenders/install/web_generator_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,32 @@ class WebGeneratorTest < Rails::Generators::TestCase
end
end

test "evaluates supported Node versions correctly" do
web_generator = Generators::Install::WebGenerator.new

unsupported_versions = %w[1.0.0 1.100.200 10.0.0 19.0.0 19.9.9 19.9999.99999]

unsupported_versions.each do |unsupported_version|
Generators::Install::WebGenerator.any_instance.stubs(:node_version).returns(unsupported_version)

assert_predicate web_generator, :node_version_unsupported?, "Node version #{unsupported_version} should not be supported"
end

supported_versions = %w[20.0.0 20.1.0 20.100.200 50.0.0 100.0.0]

supported_versions.each do |supported_version|
Generators::Install::WebGenerator.any_instance.stubs(:node_version).returns(supported_version)

assert_not_predicate web_generator, :node_version_unsupported?, "Node version #{supported_version} should be supported"
end
end

private

def prepare_destination
touch "Gemfile"

File.write("test/dummy/Gemfile", 'source "https://rubygems.org"')
end

def restore_destination
Expand Down