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

Added type checks for cpan authors #744

Merged
merged 1 commit into from Jul 30, 2014
Merged

Added type checks for cpan authors #744

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
11 changes: 9 additions & 2 deletions lib/fpm/package/cpan.rb
View file
Open in desktop
Expand Up @@ -82,8 +82,15 @@ def input(package)
self.name = fix_name(metadata["name"])
end

# Not all things have 'author' listed.
self.vendor = metadata["author"].join(", ") unless metadata["author"].nil?
# author is not always set or it may be a string instead of an array
unless metadata["author"].nil?
if metadata["author"].respond_to?(:to_str)
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather have this check if it's a String, instead of if it responds to :to_str; the contract (if there even is such a thing) on what to_s means vs to_str is not really clear in the docs and certainly not clear to me as a reader :P

I'll change this post-merge.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I actually wasn't sure if I should check for the type or the method. :)

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think there's any strict rule to play here, Ruby's a very flexible language in this respect. My personal habit is to either check for type or check for a method. Oddly, I've never seen this 'to_ary' and 'to_str' methods used in 7 years of ruby programming. Weird! :P

Copy link
Owner

Choose a reason for hiding this comment

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

For my own learning -

Poking around String#to_str has really poor docs "returns the receiver" as does Array#to_ary which says "returns self".

Wondering what else has to_ary I asked for Enumerator#to_ary docs, and this method doesn't exist. Range#to_ary also does not exist.

Based on this, I think to_str and to_ary are weird, unclear, and we should not use them.

I'll merge as-is and switch to using type checks.

Copy link

Choose a reason for hiding this comment

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

I do believe that in most cases multiple ways of getting the object type will work.
As for this...response_to? makes it (imho) more duck-type-like than just checking the type of object.
You dont really get advantages from respond_to? in this case.
Nevertheless to_str and to_ary are no methods, they are symbols for the respond_to? method.

self.vendor = metadata["author"]
elsif metadata["author"].respond_to?(:to_ary)
self.vendor = metadata["author"].join(", ")
end
end

self.url = metadata["resources"]["homepage"] rescue "unknown"

# TODO(sissel): figure out if this perl module compiles anything
Expand Down