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

Fixed crash if a .gemspec has a string for require_paths variable. #904

Closed
wants to merge 2 commits into from
Closed

Conversation

danielpclark
Copy link
Contributor

Addresses open issue "require_path from gemspec is type sensitive. #890".

@@ -134,7 +134,7 @@ def full_name
# activated.

def full_require_paths
full_paths = @require_paths.map do |path|
full_paths = [@require_paths].flatten.map do |path|
Copy link
Member

Choose a reason for hiding this comment

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

Why not use Array(@require_paths) instead?

a = %w(lib ext) #=> ["lib", "ext"]
b = "lib"
Array(a) #=> ["lib", "ext"]
Array(b) #=> ["lib"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right that's much nicer. I found two tests that had the wrong variable require_path instead of require_paths set; require_path isn't used. I'll post the chages here in the pull request.

@luislavena
Copy link
Member

@danielpclark thank you for your pull request.

Would you mind adding a test to verify this fix?

Thank you.

@danielpclark
Copy link
Contributor Author

I've fixed an existing test. Changed require_path to require_paths since require_path doesn't actually get used. Without the Array(@require_paths) fix the tests would fail. Thank you for your nicer looking code tip @luislavena .

@drbrain
Copy link
Member

drbrain commented May 8, 2014

I'm afraid the changes to the implementation may re-break elsewhere if @require_paths is used.

Instead, can you replace the attr_accessor :require_paths with a method that always returns an Array and remove use of @require_paths in both basic_specification.rb and specification.rb

@drbrain drbrain added this to the 2.3 milestone May 8, 2014
@zzak zzak closed this in 4e5f3e3 May 9, 2014
zzak pushed a commit to zzak/rubygems that referenced this pull request May 9, 2014
zzak pushed a commit to zzak/rubygems that referenced this pull request May 9, 2014
@danielpclark
Copy link
Contributor Author

@zzak

Shouldn't attr_writer :require_paths in specifications.rb now be

def require_paths= paths
  @require_paths = Array(paths)
end

per @drbrain's suggestion.

And

def require_path= path
  self.require_paths = [path]
end

should now be

def require_path= path
  self.require_paths = Array(path)
end

to avoid getting an array within an array with [] to [[]]?

I've submitted it to commit #909

@luislavena
Copy link
Member

to avoid getting an array within an array with [] to [[]]?

$ irb
irb(main):001:0> a = "lib"
=> "lib"
irb(main):002:0> b = ["lib"]
=> ["lib"]
irb(main):003:0> c = Array(a)
=> ["lib"]
irb(main):004:0> d = Array(b)
=> ["lib"]
irb(main):005:0>

@danielpclark
Copy link
Contributor Author

Yes sir. You showed that to me earlier when I used the .flatten method. It is the right way to do it. I had later seen it in one of Avdi Grimm's presentations.

drbrain added a commit that referenced this pull request May 13, 2014
Add #909 to History
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants