Adds separate configs for attributes and parameters by d-Pixie · Pull Request #1 · accodeing/rest-easy

d-Pixie · 2026-03-25T14:15:02Z

Both the checkbiz and tradera gems needed to implement a workaround for attributes in the JSON being of a different form than the query parameters. To facilitate this in the base gem a new config option conversions with two sub keys: query_parameters and json_attributes that work the same as the old attribute_conversion setting, but for the two different cases.

Copilot

Pull request overview

Introduces a new conversions configuration (with json_attributes and query_parameters) to support APIs that use different naming conventions for JSON bodies vs query parameters, while deprecating the old attribute_convention setting.

Changes:

Add nested conversions config to module settings and per-Resource config, plus new json_attribute_converter / query_parameter_converter resolution.
Automatically transform Resource.get(params: ...) query parameter keys using the configured query_parameters convention.
Update specs/docs, add a conversions spec suite, add changelog entry, and bump gem version to 1.1.0.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
spec/rest_easy/resource_spec.rb	Updates many specs to explicitly set `conversions.json_attributes` so existing PascalCase expectations still pass.
spec/rest_easy/resource/inheritance_spec.rb	Updates inheritance test to use `config.conversions.json_attributes`.
spec/rest_easy/resource/http_spec.rb	Sets module config for `conversions.json_attributes` in HTTP integration specs.
spec/rest_easy/resource/crud_spec.rb	Updates CRUD spec module config to use `conversions.json_attributes`.
spec/rest_easy/resource/conversions_spec.rb	Adds new coverage for module-level/resource-level overrides, independence of the two conversions, and deprecation behavior.
lib/rest_easy/version.rb	Bumps gem version to `1.1.0`.
lib/rest_easy/settings.rb	Adds `conversions` settings and deprecates `attribute_convention` at the module level.
lib/rest_easy/resource.rb	Implements converter resolution and applies query parameter conversion in `get`.
lib/rest_easy.rb	Adds BC propagation from deprecated `attribute_convention` to `conversions.json_attributes`.
README.md	Updates documentation to the new `conversions.*` configuration and adds query-params behavior notes.
Gemfile.lock	Updates path gem version to `1.1.0`.
CHANGELOG.md	Adds `1.1.0` entry documenting conversions and deprecation.

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

Copilot · 2026-03-26T05:49:18Z

+### Deprecated
+
+- **`attribute_convention`** is deprecated in favour of `conversions.json_attributes`. The old setting continues to work and is respected as a fallback, but emits a deprecation warning when used at the Resource level. Module-level `attribute_convention` is silently supported for backwards compatibility.
+


The changelog says module-level attribute_convention is "silently supported" for backwards compatibility, but RestEasy.configure currently emits a deprecation warning when it sees attribute_convention set. Either update the changelog wording or adjust the implementation to avoid warning at the module level (and only warn for resource-level usage).

ehannes

Looks solid. My concern is more about code quality and structure, which mostly is out of scope for this PR. But the Resource class is really large and does a lot. Why not split that into several classes?

I would also like to change things regarding the test setup to make it more robust. I think we are leaking setup code between test cases. We should start using Rubocop, I'm pretty sure we are violating a lot of rules :)

ehannes · 2026-03-26T05:51:41Z

+  def setup_test_connection(api_module, &block)
+    stubs = Faraday::Adapter::Test::Stubs.new(&block)
+    api_module.instance_variable_set(:@faraday_connection, nil)
+    api_module.connection do |f|
+      f.request :json
+      f.response :json, content_type: /\bjson$/
+      f.adapter :test, stubs
+    end
+    stubs
+  end


This looks like something that should be extracted to a spec helper. The spec is already really long.

ehannes · 2026-03-26T05:52:46Z

+    before(:all) do
+      module ConvTestApi
+        extend RestEasy
+
+        configure do
+          conversions.json_attributes = :PascalCase
+          conversions.query_parameters = :camelCase
+        end
+      end
+
+      class ConvTestApi::Invoice < RestEasy::Resource
+        configure { path "invoices" }
+
+        key :document_number, Integer, :read_only
+        attr :customer_name, String
+      end
+    end


Are we using Rubocop in this project? I'm pretty sure this will be flagged as leaking test setup. We should stub the class in stead of creating a mock module. We have established patterns for this in other projects.

Let's fix this later...

ehannes · 2026-03-26T05:53:08Z

+    after(:all) do
+      Object.send(:remove_const, :ConvTestApi)
+    end


And you will get rid of this if you use stubbing instead.

ehannes · 2026-03-26T05:55:43Z

+    stubs
+  end
+
+  # ── Module-level configuration ──────────────────────────────────────


These comments, I'm not a fan of comments in code and especially not block level comments like these. Most often they will be outdated. Also - we already have the describe blocks.

ehannes · 2026-03-26T05:59:03Z

      end

-      # -- attribute_convention ------------------------------------------
+      # -- conversions ---------------------------------------------------


What happened with small single responsibility classes? This class is huge. Also I'm not a fan of these comments. If this is a separate concern in this class then it might be a sign that we can extract it to a separate class that we can test in isolation 😄

ehannes · 2026-03-26T06:01:40Z

    describe "path" do
      it "sets the endpoint path via configure" do
        resource = Class.new(described_class) do
+          configure { conversions.json_attributes = :PascalCase }


This is why I dislike the test strategy where you inline everything in one it statement. As soon as you change something, like this configuration, you need to update every test for that class (which in this case is very many!). I suggest using a shared setup instead.

ehannes · 2026-05-15T06:26:09Z

Regarding all suggested code improvements from me - let's do that later, none of it is actually related to this pr...

Copilot

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.

+        ac = self::Settings.config.attribute_convention
+        if ac && @_propagated_attribute_convention != ac
+          warn "RestEasy: attribute_convention is deprecated, use `conversions.json_attributes = #{ac.inspect}` instead"
+          self::Settings.config.conversions.json_attributes = ac


The gem never references Dry::Inflector — Zeitwerk's own inflector handles the .inflect call. The pin to ~> 0.2.1 also conflicted with dry-types >= 1.7, which requires dry-inflector ~> 1.0.

Replaces transform_keys! with the non-mutating transform_keys so callers can reuse or freeze the params hash they pass to get.

Memoises the last-propagated value so subsequent configure calls don't clobber an explicitly set conversions.json_attributes (and don't repeat the deprecation warning).

Restores the 1.0.0 default of :PascalCase (formerly via attribute_convention) for both json_attributes and query_parameters. Keeps the two settings symmetric so upgrading users see no behavioural change without an explicit major version bump.

The implementation emits a deprecation warning at the module level too, not just at the resource level. Update the wording to reflect that.

Memoisation made later configure calls invisible to resources that had already resolved a converter. Resolving on every call is cheap (a frozen hash lookup) and removes the staleness risk entirely. Adds an explicit :PascalCase fallback so a resource with no parent RestEasy module and no per-resource conversions configured still resolves to a usable converter instead of nil.

Removes 64 now-redundant `configure { conversions.json_attributes = :PascalCase }` setters from the resource spec — they only existed to preserve PascalCase behaviour while the default was briefly :snake_case and became no-ops once the default returned to :PascalCase. Replaces two internal uses of the deprecated klass.attribute_convention with klass.json_attribute_converter, factors the :PascalCase literal into a Conventions::DEFAULT constant, spells out the "BC" comment shorthand, drops the now-default conversions setters from the README examples, and notes the loss of the attribute_convention default in the changelog.

d-Pixie requested a review from ehannes March 25, 2026 14:15

d-Pixie self-assigned this Mar 25, 2026

d-Pixie added the enhancement New feature or request label Mar 25, 2026

ehannes requested a review from Copilot March 26, 2026 05:44

Copilot started reviewing on behalf of ehannes March 26, 2026 05:44 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

ehannes requested changes Mar 26, 2026

View reviewed changes

ehannes force-pushed the development branch 2 times, most recently from 638994f to 5d9ce10 Compare May 15, 2026 06:24

ehannes requested a review from Copilot May 15, 2026 06:24

Copilot started reviewing on behalf of ehannes May 15, 2026 06:25 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

d-Pixie and others added 13 commits May 15, 2026 08:48

Adds separate configs for attributes and parameters

8b4f8d5

Updates README.md with new config structure

e2a113f

Moves resolver defaults to parent module settings

137c784

Proxies old module setting to new config

df5b93c

Simplifies the memoized converter functions

493f69c

Simplifies query param conversion logic

35d8512

Drops the unused dry-inflector dependency

545bf98

The gem never references Dry::Inflector — Zeitwerk's own inflector handles the .inflect call. The pin to ~> 0.2.1 also conflicted with dry-types >= 1.7, which requires dry-inflector ~> 1.0.

Fixes Resource.get mutating caller-provided params hash

bab470f

Replaces transform_keys! with the non-mutating transform_keys so callers can reuse or freeze the params hash they pass to get.

Stops re-propagating attribute_convention to conversions

786adaa

Memoises the last-propagated value so subsequent configure calls don't clobber an explicitly set conversions.json_attributes (and don't repeat the deprecation warning).

Reconciles changelog wording on attribute_convention

42fdd74

The implementation emits a deprecation warning at the module level too, not just at the resource level. Update the wording to reflect that.

ehannes force-pushed the development branch from 2a54991 to 5437d3a Compare May 15, 2026 06:49

ehannes merged commit 760db45 into main May 15, 2026
1 check passed

		### Deprecated

		- `attribute_convention` is deprecated in favour of `conversions.json_attributes`. The old setting continues to work and is respected as a fallback, but emits a deprecation warning when used at the Resource level. Module-level `attribute_convention` is silently supported for backwards compatibility.

Conversation

d-Pixie commented Mar 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ehannes left a comment

Choose a reason for hiding this comment

Uh oh!

ehannes Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

ehannes Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

ehannes May 15, 2026

Choose a reason for hiding this comment

Uh oh!

ehannes Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

ehannes Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

ehannes Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

ehannes Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

ehannes commented May 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants