Change Grape::API's @setup var to an Array (from a Set)
#2529
Conversation
|
I like it, @ericproulx any comments/objections? |
There was a problem hiding this comment.
I'm not sure if creating a test with Set here makes sense. It could test only the expected behavior, and not a code that will be removed from the codebase.
There was a problem hiding this comment.
I kinda agree but that was what was asked so..
There was a problem hiding this comment.
@Haerezis I think we misunderstood each other :)
@stephannv is right, the only test we need is one that would fail without the change in lib/grape/api.rb. So you can remove SetupSetAPI and SetupArrayAPI, reduce the test to a minimum that fails without the change to = []. Maybe even add the scenario to an existing _spec.rb.
There was a problem hiding this comment.
Ha, sorry, we must have effectively misunderstood each other.
So in short it's not even a test changing anything about @setup, it's just a test checking that mounted routes have the correct number of settings and that none are missing. And this test would fail with the current Impl using a Set, but because I would also change the current impl to use an Array, it would NOT fail.
Sorry I was confused with the wording of a test that fails without the change to = []. I understood it as "show us a comparison test because we don't accept the change without the comparison".
At least the current spec have shown beyond the shadow of a doubt that using a Set introduce a bug.
I'll come back quickly with what you asked.
There was a problem hiding this comment.
Exactly, English is my third language ;) Thank you!
There was a problem hiding this comment.
@Haerezis I think we misunderstood each other :)
@stephannv is right, the only test we need is one that would fail without the change in lib/grape/api.rb. So you can remove SetupSetAPI and SetupArrayAPI, reduce the test to a minimum that fails without the change to = []. Maybe even add the scenario to an existing _spec.rb.
|
@dblock @stephannv I've redone the specs. Remove old file, added the tests to api_remount_spec.rb (seems like the right place for it). |
CHANGELOG.md
Outdated
| @@ -11,6 +11,7 @@ | |||
| * [#2514](https://github.com/ruby-grape/grape/pull/2514): Add rails 8.0 to CI - [@ericproulx](https://github.com/ericproulx). | |||
| * [#2516](https://github.com/ruby-grape/grape/pull/2516): Dynamic registration for parsers, formatters, versioners - [@ericproulx](https://github.com/ericproulx). | |||
| * [#2518](https://github.com/ruby-grape/grape/pull/2518): Add ruby 3.4 to CI - [@ericproulx](https://github.com/ericproulx). | |||
| * [#2529](https://github.com/ruby-grape/grape/pull/2529): Change Grape::API's `@setup` var to an array (from a set) - [@Haerezis](https://github.com/Haerezis). | |||
There was a problem hiding this comment.
This is actually a bug fix, should go below under fixes with something like "Fix route_setting used with multiple mount".
There was a problem hiding this comment.
I changed the changelog and put the change in fixes section with a title I thought fit better.
Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>
Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>
|
Merci @Haerezis! |
|
my pleasure. Sorry for the misunderstanding :) Thanks for the quick replies ! |
A Set lead Grape to ignore identical DSL calls when mounting an API, as they were not recorded when the API was first loaded (because identical item are ignored when added to a Set).
#2173