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

Grape 2.2.0 compatibility #940

Merged
merged 5 commits into from
Sep 21, 2024
Merged

Conversation

padde
Copy link
Contributor

@padde padde commented Sep 16, 2024

Running on grape 2.0.2 resulted in the following exceptions:

NameError:
  uninitialized constant Grape::ContentTypes::CONTENT_TYPES
# ./lib/grape-swagger/endpoint.rb:15:in `content_types_for'

NoMethodError:
  undefined method `formatters' for module Grape::Formatter
# ./lib/grape-swagger/endpoint.rb:14:in `content_types_for'

This is due to internal refactorings in grape as of v2.2.0, specifically ruby-grape/grape@fb67ea99

Fixes #939

@padde padde changed the title Grape 2.0.2 compatibility Grape 2.2.0 compatibility Sep 16, 2024
README.md Outdated Show resolved Hide resolved
grape-swagger.gemspec Outdated Show resolved Hide resolved
@padde padde force-pushed the grape-2.0.2-compatibility branch 3 times, most recently from 296bda1 to 8d98422 Compare September 16, 2024 16:28
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Instead of relying on a Grape constant that's not supposed to be part of the public interface, just copy it into grape-swagger? Expand CI to use multiple versions of Grape to make sure our compatibility is tested well.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
grape-swagger.gemspec Outdated Show resolved Hide resolved
@padde
Copy link
Contributor Author

padde commented Sep 17, 2024

Instead of relying on a Grape constant that's not supposed to be part of the public interface, just copy it into grape-swagger? Expand CI to use multiple versions of Grape to make sure our compatibility is tested well.

@dblock sure, we could do that and it would vastly simplify the implementation, as we wouldn't have to massage the internal constants as much to make it work for our purposes here.

@padde
Copy link
Contributor Author

padde commented Sep 17, 2024

@dblock looking further into the code that is used from grape here, it seems that there always has been a flaw in the way we retrieve content types in grape-swagger. Grape allows adding user-defined content types. However, if I get this right, grape-swagger never even considered the ones coming from the settings, just the default values from the internal constants that are now causing the issue. Let's continue discussion in #939 (comment)

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

The way you implemented this create a lot of repetition for the variable. Can you try and please turn this into a matrix with two entries: a ruby version and a grape version? We don't need to test all possible permutations either. Here's an example of one: https://github.com/mongoid/mongoid-locker/blob/master/.github/workflows/test.yml

And to be clear, we should merge this and do a release so users aren't more broken than before, so appreciate your work here!

@padde
Copy link
Contributor Author

padde commented Sep 19, 2024

Can you try and please turn this into a matrix with two entries: a ruby version and a grape version?

@dblock sure. Honestly, I just expanded on the existing approach to avoid this discussion but I am happily reducing the duplication!

We don't need to test all possible permutations either.

That is a bit vague - which permutations should we test then, if not all? I will for now follow the "wasteful" approach with all permutations, feel free to tell me which ones to remove, if you like.

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.

Compatibility with grape 2.2.0
2 participants