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

Fixes (restores) route ordering #859

Merged
merged 6 commits into from
Mar 3, 2024
Merged

Conversation

dchandekstark
Copy link
Contributor

This restores the route ordering which was changed in #631.

Fixes #679

@dchandekstark
Copy link
Contributor Author

I have one failing test, but I have to wonder if it's due to a bug in grape. The purported solution in #631 may have simply masked the problem. If in fact the issue is with grape-swagger I don't think the appropriate solution is to simply reverse the order of the route set. In any event, the test case seems to me rather odd. I couldn't find anything quite like it in grape's docs or specs (ofc I could well have missed it).

be a blocker to making the changes in this PR.

While there does not appear to be a grape test with the same setup,
the spec at https://github.com/ruby-grape/grape/blob/e3451c892abd82c15f0937d7f464613ae715a73d/spec/grape/api_spec.rb#L1025-L1045
suggests that mounts do not overwrite.
@dchandekstark
Copy link
Contributor Author

Please note that this change may reintroduce the issue that #631 attempted to solved, albeit IMO incorrectly.

@dchandekstark
Copy link
Contributor Author

I know that I have dodged a problem by removing a test (and I haven't yet tried adding it back), but this route ordering problem still exists. I love this project, and I'd like to help fix this. I have merged in the latest changes and all other tests are passing.

@LeFnord
Copy link
Member

LeFnord commented Mar 3, 2024

thanks for finishing it
ok will merge it, will see if the ordering discussion pops up again ;)

@LeFnord LeFnord merged commit ccdc2dd into ruby-grape:master Mar 3, 2024
21 checks passed
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.

Grape swagger shows operations in reverse order.
2 participants