KimNorgaard / validates_hostname

Extension to ActiveRecord::Base for validating hostnames and domain names
MIT License
38 stars 12 forks source link

Don't include unnecessary requirements in gemspec + better error messages #2

Closed sshaw closed 9 years ago

sshaw commented 9 years ago

Hi, a few things here:

Your gemspec included a bunch of stuff that one doesn't need when using this library. For example, my project uses MySQL, but your gem installed the sqlite3 driver :crying_cat_face: This should fix things up.

The gemspec line in your Gemfile will load the dependencies defined in your gemspec. No need to repeat them in there.

The doc directory is not needed. The user of your library may choose install this as needed.

.rspec: nested format was changed to documentation.

I also changed the validation messages. For example:

class User < ActiveRecord::Base
  validates :website, :hostname => { :require_valid_tld => true, :allow_blank => true }
end

If website is invalid one will receive something like: Website hostname must be between 1 and 255 characters long or Website label begins or ends with hyphen

If the form field is named "Website" then "Website hostname", "Website label", etc... are confusing as all the user knows/sees is "Website".

sshaw commented 9 years ago

Also, may be good to add something like to this to automatically grab all the (valid?) top-level domains:

io = open(ROOT_DB)
doc = Nokogiri::HTML(io)
doc.search("td .tld").each do |e|
  puts e.text if e.text =~ %r(\A\.[[:ascii:]]+\Z)
end
io.close
KimNorgaard commented 9 years ago

Thanks a lot for the PR. Apparently I created this with my head up my ass :-) Anyway, it's got some size so I'll need some time to look it through before i merge it in.

sshaw commented 9 years ago

My commit removed the "label" part from some error messages, but on further review I think that the following should be changed again as something is needed to disambiguate it from the whole. "Label" is confusing:

"label must be between 1 and 63 characters long" and "label begins or ends with hyphen".

May be better to use "a domain" instead of label?

KimNorgaard commented 9 years ago

I'm quite content witt label. I try use the same terminology as defined by https://www.ietf.org/rfc/rfc1035.txt and friends. Label is the correct term. Domain would be ambiguous.

sshaw commented 9 years ago

I'm quite content witt label. I try use the same terminology as defined by https://www.ietf.org/rfc/rfc1035.txt

As do I (:neckbeard:) but, when in comes to most users, they don't know RFC from RPG and if someone enters x.co.uk -where x > 63 my concern is the message "label must be between 1 and 63 characters long" will not evince things.

But really that's why one can set their own messages when defining validators. So no big deal really :smile: