ostinelli / apnotic

A Ruby APNs HTTP/2 gem able to provide instant feedback.
MIT License
480 stars 96 forks source link

Priority should not need to be a string #23

Closed schmidp closed 8 years ago

schmidp commented 8 years ago

n = Apnotic::Notification.new(token) n.priority = 10.to_s

It would be nicer if you could just write: n.priority = 10

ostinelli commented 8 years ago

I don't see any specs that apns-priority should be a string here, do you have any specifics in mind where it is clearly stated that it needs to be a string?

mat commented 8 years ago

Hi, I think what @schmidp is seeing is that n.priority = 5 gives

HTTP2::Error::CompressionError(undefined method `bytesize' for 5:Fixnum
Did you mean?  bytes):
  http-2 (0.8.2) lib/http/2/compressor.rb:289:in `size_check'
  http-2 (0.8.2) lib/http/2/compressor.rb:278:in `add_to_table'
  http-2 (0.8.2) lib/http/2/compressor.rb:182:in `process'
  http-2 (0.8.2) lib/http/2/compressor.rb:206:in `block in encode'
  http-2 (0.8.2) lib/http/2/compressor.rb:202:in `each'
  http-2 (0.8.2) lib/http/2/compressor.rb:202:in `encode'
  http-2 (0.8.2) lib/http/2/compressor.rb:449:in `encode'
  http-2 (0.8.2) lib/http/2/connection.rb:593:in `encode_headers'
  http-2 (0.8.2) lib/http/2/connection.rb:370:in `encode'
  http-2 (0.8.2) lib/http/2/connection.rb:356:in `send'
  http-2 (0.8.2) lib/http/2/client.rb:38:in `send'
  http-2 (0.8.2) lib/http/2/emitter.rb:34:in `block in emit'
  http-2 (0.8.2) lib/http/2/emitter.rb:33:in `delete_if'
  http-2 (0.8.2) lib/http/2/emitter.rb:33:in `emit'
  http-2 (0.8.2) lib/http/2/stream.rb:141:in `send'
  http-2 (0.8.2) lib/http/2/stream.rb:157:in `headers'
  net-http2 (0.13.3) lib/net-http2/stream.rb:86:in `send_request_data'
  net-http2 (0.13.3) lib/net-http2/stream.rb:26:in `call_with'
  net-http2 (0.13.3) lib/net-http2/client.rb:27:in `call'
  apnotic (0.10.2) lib/apnotic/connection.rb:32:in `push'

while n.priority = "5" doesn't give an error and seems to work. I've noticed this when trying to use n.expiration = 12345, which also fails like this.

I agree that n.priority = <integer> seems like the more intuituve interface to me here, @ostinelli would you like me to create a PR to change that?

ostinelli commented 8 years ago

Fixed in 0bcd8a4632ae9e945341d878fb7beb88a8e4a5cf, thank you for your help.