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

[xcodes] fix issue where xcodes action wouldn't accept beta versions of Xcode #21434

Merged
merged 1 commit into from
Aug 28, 2023
Merged

[xcodes] fix issue where xcodes action wouldn't accept beta versions of Xcode #21434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 0 additions & 3 deletions fastlane/lib/fastlane/helper/xcodes_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@ def self.find_xcodes_binary_path
module Verify
def self.requirement(req)
UI.user_error!("Version must be specified") if req.nil? || req.to_s.strip.size == 0
Comment on lines 19 to 20
Copy link
Collaborator

Choose a reason for hiding this comment

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

The requirement method was only used in this one place: the verify_block in the xcodes action, as far as I can tell. I think if we are to remove it, it would probably make sense to remove the whole thing and have it as an inline block in the action's ruby file, which matches other verify_block methods. This one was probably pulled out because it was a bit longer and using a rescue block, but generally 1-2 line verification methods are inline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is still used in two places, therefore I left as it was for reuse

Screenshot 2023-08-05 at 21 23 04

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I see. So shouldn't xcversion continue to use the old validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would rather let xcodes or xcversion let decide to throw an error, fastlane should only pre check for empty strings

Copy link
Member

Choose a reason for hiding this comment

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

I think I agree with @apps4everyone, the underlying tools will error out if the user enter an invalid version. I think that checking just for string validity (empty/whitespaces) is what makes more sense here 👌

Gem::Requirement.new(req.to_s)
rescue Gem::Requirement::BadRequirementError
UI.user_error!("The requirement '#{req}' is not a valid RubyGems style requirement")
end
end
end
Expand Down
10 changes: 0 additions & 10 deletions fastlane/spec/actions_specs/xcversion_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,6 @@
double("XcodeInstall::Xcode", version: "7.3", path: "/Test/Xcode7.3")
end

context "with an invalid requirement" do
it "raises an error" do
expect do
Fastlane::FastFile.new.parse("lane :test do
xcversion version: '= aaaa'
end").runner.execute(:test)
end.to raise_error("The requirement '= aaaa' is not a valid RubyGems style requirement")
end
end

context "with a valid requirement" do
before do
require "xcode/install"
Expand Down
16 changes: 0 additions & 16 deletions fastlane/spec/helper/xcodes_helper_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,22 +45,6 @@
expect { subject.class::Verify.requirement(argument) }.to raise_error(FastlaneCore::Interface::FastlaneError, 'Version must be specified')
end
end

context "when argument is not in RubyGems style" do
let(:argument) { "banana" }

it "raises user error" do
expect { subject.class::Verify.requirement(argument) }.to raise_error(FastlaneCore::Interface::FastlaneError, "The requirement '#{argument}' is not a valid RubyGems style requirement")
end
end

context "when argument is in RubyGems style" do
let(:argument) { "1.2.3" }

it "returns Gem::Requirement instance" do
expect(subject.class::Verify.requirement(argument)).to be_an_instance_of(Gem::Requirement)
end
end
end
end
end