Skip to content

feat!: reuse TCP connection by default#49

Open
figueredo wants to merge 1 commit into
mainfrom
feat/keep-alive-true
Open

feat!: reuse TCP connection by default#49
figueredo wants to merge 1 commit into
mainfrom
feat/keep-alive-true

Conversation

@figueredo
Copy link
Copy Markdown
Contributor

Change TCP connection reuse to be default behavior so that clients have better performance with less configuration.

@figueredo figueredo self-assigned this May 15, 2026
Copilot AI review requested due to automatic review settings May 15, 2026 19:24
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR changes the client’s default HTTP behavior to reuse TCP connections (persistent connections) without requiring explicit keep_alive: true, improving performance with reduced configuration.

Changes:

  • Switch Configuration#configure default keep_alive from false to true.
  • Update specs to reflect the new default and validate adapter selection.
  • Update README to reflect that connection reuse is enabled by default and simplify the max_connections example.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
lib/incognia_api/configuration.rb Changes default keep_alive to true, making persistent connections the default.
spec/incognia_spec.rb Updates configuration specs to assert keep_alive defaults to true and adjusts the max_connections/keep_alive validation scenario.
spec/client_spec.rb Adds a spec asserting the persistent adapter is selected by default.
README.md Updates documentation to state connection reuse is enabled by default and removes keep_alive: true from the example.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread README.md Outdated
Comment thread spec/client_spec.rb
@figueredo figueredo force-pushed the feat/keep-alive-true branch from fdd151b to 7de4389 Compare May 15, 2026 21:45
@figueredo figueredo changed the title feat: reuse TCP connection by default feat!: reuse TCP connection by default May 15, 2026
Copilot AI review requested due to automatic review settings May 15, 2026 21:48
@figueredo figueredo force-pushed the feat/keep-alive-true branch from 7de4389 to 64b91ec Compare May 15, 2026 21:48
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

lib/incognia_api.rb:38

  • format_message can return nil when errors is nil because the conditional append is the last expression in the method. That makes APIError#message/to_s potentially nil for cases like timeouts (where response_info is {}), which is surprising and can break consumers expecting a string. Consider explicitly returning message at the end of format_message (or using tap) so it always returns a String.
    def initialize(message, response_info = {})
      response_info ||= {}

      @status = response_info[:status]
      @errors = response_info[:body]
      @message = format_message(message)
    end

Copy link
Copy Markdown
Member

@ottony ottony left a comment

Choose a reason for hiding this comment

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

Can you bump the version, update the lock, and CHANGELOG, please?

https://github.com/inloco/incognia-ruby#development

update the version number in version.rb, run bundle to update the gemfile.lock, add one description on CHANGELOG.md if necessary

Comment thread lib/incognia_api.rb
Comment on lines 32 to +33
def initialize(message, response_info = {})
response_info ||= {}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Was it sent as null?
As you're already treating nil, we can change the default to be nil, wdyt?

Suggested change
def initialize(message, response_info = {})
response_info ||= {}
def initialize(message, response_info = nil)
response_info ||= {}

attr_accessor :client_id, :client_secret, :host, :keep_alive, :max_connections

def configure(client_id:, client_secret:, host: nil, keep_alive: false, max_connections: nil)
def configure(client_id:, client_secret:, host: nil, keep_alive: true, max_connections: nil)
Copy link
Copy Markdown
Member

@ottony ottony May 20, 2026

Choose a reason for hiding this comment

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

Is it a breaking change, or is it okay to assume it as the default?

I'm asking so we can decide about the version.

Comment thread spec/client_spec.rb
host: 'https://api.incognia.com/api'
)

allow_any_instance_of(Faraday::RackBuilder).to receive(:adapter).and_call_original
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did we need to mock it before the expect_any_instance_of? Does it have another call, with different params?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants