search

KimNorgaard / validates_hostname

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

allow for conditional validation, controlled with model method #16

Closed krtschmr closed 1 year ago

krtschmr commented 3 years ago

we need to have valid tlds, but we also need to allow .local tld.

this PR opens functionality to have a validation like this


validates :base_domain, presence: true, hostname: { require_valid_tld: :valid_tld_required? }

  def valid_tld_required?
    !base_domain.end_with?(".local")
  end
krtschmr commented 2 years ago

rip open source development :/

KimNorgaard commented 2 years ago

Sorry, I sort of ignored the request because I was thinking about whether the solution provided was what I wanted and then forgot about it :)

The gem was meant to be very simple. Adding conditionals like the one proposed adds feature creep which I would like to avoid. There is already and option :valid_tlds that could be used for the purpose, right?

krtschmr commented 1 year ago

yes, valid_tlds could be used. so maybe then valid_tlds: ALLOWED_TLDS.merge("local"). This would then allow to have a .local domain. Our usecase is different. We need to skip the validation, if we use a $client.local.domain.com domain, which we use for development.

I believe conditional logic shall always be supported in any rails environment. Since you're the owner, you're in charge. So there's nothing more than I can do. I can tell you, my PR is running in production for more than a year and is doing good.

KimNorgaard commented 1 year ago

Fair enough. I see how this PR increases flexibility. I'll merge.

krtschmr commented 1 year ago

🥳 awesome. now we can use your master again!

krtschmr commented 1 year ago

Also @KimNorgaard on my private personal level: Thank you for rethinking your position. I have bad experience in the open source world where people just block development of "their" gems because of personal bias/mission/vision. Many PRs in this world (not just mine) are stale forever because of those.

Kudos to you!

KimNorgaard commented 1 year ago

@krtschmr no problem and I agree. I'll admit I'm becoming increasingly nervous every time I see a PR on this gem because I don't use ruby or rails anymore and haven't for almost 10 years. It was originally written for rails 2 but now we're on .. 7? I know it has been a dependency in some fairly big projects (gitlab comes to mind) and I'm simply afraid of what I'll break if I change things. Your PR seemed to not break backwards compatibility though. Thanks for contributing :)