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

(#82) fix undefined method .blank? #84

Merged
merged 1 commit into from
Feb 19, 2024
Merged

Conversation

bastelfreak
Copy link
Contributor

This adds the activesupport dependency which provides .blank?. It's used in a lot of places so I added the dependency instead of switching to native ruby.

$ git grep .blank?
lib/hcloud/certificate_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/certificate_resource.rb:        raise Hcloud::Error::InvalidInput, 'no certificate given' if certificate.blank?
lib/hcloud/certificate_resource.rb:        raise Hcloud::Error::InvalidInput, 'no private_key given' if private_key.blank?
lib/hcloud/entry_loader.rb:          raise Hcloud::Error::InvalidInput, 'no type given' if kwargs[:type].blank?
lib/hcloud/entry_loader.rb:          raise Hcloud::Error::InvalidInput, 'no start given' if kwargs[:start].blank?
lib/hcloud/entry_loader.rb:          raise Hcloud::Error::InvalidInput, 'no end given' if kwargs[:end].blank?
lib/hcloud/firewall_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/floating_ip.rb:      raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/floating_ip_resource.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if type.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no dns_ptr given' if dns_ptr.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if load_balancer_type.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if type.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no protocol given' if protocol.blank?
lib/hcloud/load_balancer.rb:        raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/load_balancer_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/load_balancer_resource.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if load_balancer_type.blank?
lib/hcloud/load_balancer_resource.rb:      if !algorithm.to_h.key?(:type) || algorithm[:type].blank?
lib/hcloud/load_balancer_resource.rb:      if location.blank? && network_zone.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if type.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no network_zone given' if network_zone.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no ip_range given' if ip_range.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no destination given' if destination.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no gateway given' if gateway.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no destination given' if destination.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no gateway given' if gateway.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no ip_range given' if ip_range.blank?
lib/hcloud/network_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/network_resource.rb:      raise Hcloud::Error::InvalidInput, 'no IP range given' if ip_range.blank?
lib/hcloud/placement_group_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/primary_ip.rb:      raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/primary_ip_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/primary_ip_resource.rb:      raise Hcloud::Error::InvalidInput, 'no assignee_type given' if assignee_type.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no image given' if image.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no server_type given' if server_type.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no iso given' if iso.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no dns_ptr given' if dns_ptr.blank?
lib/hcloud/ssh_key_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/volume_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/volume_resource.rb:      if location.blank? && server.nil?

@ghost
Copy link

ghost commented Feb 15, 2024

I have been wondering why you set a version restriction spec.add_runtime_dependency 'activesupport', '~> 7.1', '>= 7.1.1' in the gemspec. Are previous versions of activesupport broken or somehow different?

The new line require 'active_support/core_ext/object/blank' looks fine, only the version restriction is confusing me (if it has no real reason).

@bastelfreak
Copy link
Contributor Author

As far as I learned it's good practice to use explicit pinning to ensure we don't pull in old versions that don't work and also don't pull in new major versions. 7.1.1 was the latest version when I wrote the patch.

@ghost
Copy link

ghost commented Feb 15, 2024

Hm... But doesn't the gemspec transitively expand also on downstream users of this library? I don't think we should force other projects to use only activesupport '>= 7.1.1' (unless we know for sure that our library does not work with versions below 7.1.1).

I usually like to keep libraries as broad as possible and pin applications strictly (what Gemfile.lock does in the Ruby world). In libraries I only add version restrictions if they're absolutely necessary. If libraries used version restrictions often, it could easily happen that an applications wants to use two libraries but they have incompatible version restrictions.

I have one project that uses this library and added a restriction activesupport < 7.1 there to simulate that maybe (for whatever reason) that project only works with older versions of activesupport.

With the current release of hcloud-ruby that works without problems. It pins activesupport to activesupport (7.0.8) in Gemfile.lock. If I use this PR of hcloud-ruby it leads to a version resolution error:

Could not find compatible versions

Because every version of [myapplication] depends on activesupport < 7.1
  and every version of hcloud depends on activesupport >= 7.1.1, < 8.A

If you can remove the version restriction, I could merge it.

@bastelfreak
Copy link
Contributor Author

@aufziehvogel are you fine with the upper limit to prevent pulling int major releases that might break hcloud? with the dependabot plugin you will get an automatic PR that will run tests and bump the version, so you won't miss it.

@ghost
Copy link

ghost commented Feb 16, 2024

@tonobo What do you think? I'd also allow future major versions (in this case activesupport 8 or later), because the chances that our code still runs with activesupport 8 without problems are pretty high. Or in other words, I'd only put a restriction if I really know that we're broken with a specific version.

@tonobo
Copy link
Owner

tonobo commented Feb 16, 2024

Howdy, applying a static pinning via gemspec is not what we commonly do, it's up to your project to pin dependencies (usually via Gemfile.lock). I personally dislike introducing hard dependencies at gem level, guess the project isn't maintained anymore but still enforcing
to stick to an old ruby versions. grpc gem did something like this in the past which blocked our internal upgrade procedures around christmas because everybody there was on vacation. There are definitely cons to do so, but I'd like to stick to the actual no pinning way. Maybe @Kjarrigan or @aufziehvogel might have a different opinion there.

@ghost
Copy link

ghost commented Feb 16, 2024

Same opinion from my side. I also prefer no pinnings in gemspec.

This adds the activesupport dependency which provides `.blank?`. It's used
in a lot of places so I added the dependency instead of switching to
native ruby.

```
$ git grep .blank?
lib/hcloud/certificate_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/certificate_resource.rb:        raise Hcloud::Error::InvalidInput, 'no certificate given' if certificate.blank?
lib/hcloud/certificate_resource.rb:        raise Hcloud::Error::InvalidInput, 'no private_key given' if private_key.blank?
lib/hcloud/entry_loader.rb:          raise Hcloud::Error::InvalidInput, 'no type given' if kwargs[:type].blank?
lib/hcloud/entry_loader.rb:          raise Hcloud::Error::InvalidInput, 'no start given' if kwargs[:start].blank?
lib/hcloud/entry_loader.rb:          raise Hcloud::Error::InvalidInput, 'no end given' if kwargs[:end].blank?
lib/hcloud/firewall_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/floating_ip.rb:      raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/floating_ip_resource.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if type.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no dns_ptr given' if dns_ptr.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if load_balancer_type.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if type.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no protocol given' if protocol.blank?
lib/hcloud/load_balancer.rb:        raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/load_balancer_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/load_balancer_resource.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if load_balancer_type.blank?
lib/hcloud/load_balancer_resource.rb:      if !algorithm.to_h.key?(:type) || algorithm[:type].blank?
lib/hcloud/load_balancer_resource.rb:      if location.blank? && network_zone.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if type.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no network_zone given' if network_zone.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no ip_range given' if ip_range.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no destination given' if destination.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no gateway given' if gateway.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no destination given' if destination.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no gateway given' if gateway.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no ip_range given' if ip_range.blank?
lib/hcloud/network_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/network_resource.rb:      raise Hcloud::Error::InvalidInput, 'no IP range given' if ip_range.blank?
lib/hcloud/placement_group_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/primary_ip.rb:      raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/primary_ip_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/primary_ip_resource.rb:      raise Hcloud::Error::InvalidInput, 'no assignee_type given' if assignee_type.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no image given' if image.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no server_type given' if server_type.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no iso given' if iso.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no dns_ptr given' if dns_ptr.blank?
lib/hcloud/ssh_key_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/volume_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/volume_resource.rb:      if location.blank? && server.nil?
```
@bastelfreak
Copy link
Contributor Author

@skoch-hc thanks for merging, can you also do a new release?

@skoch-hc
Copy link
Collaborator

Have released version 1.3.0 to rubygems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants