-
Notifications
You must be signed in to change notification settings - Fork 64
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
Add support hanami router #205
Conversation
…ment * Make work minitest with new extractors * Apply rubocop offences
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #205 +/- ##
==========================================
+ Coverage 97.15% 97.22% +0.06%
==========================================
Files 15 20 +5
Lines 528 612 +84
Branches 127 133 +6
==========================================
+ Hits 513 595 +82
- Misses 15 17 +2 ☔ View full report in Codecov by Sentry. |
spec/apps/hanami/doc/openapi.yaml
Outdated
@@ -212,16 +236,11 @@ paths: | |||
example: | |||
items: | |||
- secrets | |||
'401': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why 401
responses are gone?
It looks like a regression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have researched this issue, missed it unfortunately earlier. I think it's not a regression, it's just that this section of documentation in rails is manually added, maybe it's part of testing that checks that manually added documentation won't be removed automatically.
When I generated the hanami documentation I deleted the whole file and re-generated it, I will add this part manually if I understand the reason for the existence of this part correctly.
@@ -168,9 +167,31 @@ paths: | |||
schema: | |||
type: string | |||
format: binary | |||
"/images/{id}": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to myself) Before this enhancement, /images/1
and /images/2
were separately generated, which indicates that Hanami's Routing mechanism is not honored.
lib/rspec/openapi.rb
Outdated
@@ -1,5 +1,6 @@ | |||
# frozen_string_literal: true | |||
|
|||
require 'bundler/setup' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is bundler
gem always included in runtime?
If not, it should be added to add_dependency
section:
rspec-openapi/rspec-openapi.gemspec
Lines 31 to 33 in cdda8fc
spec.add_dependency 'actionpack', '>= 5.2.0' | |
spec.add_dependency 'rails-dom-testing' | |
spec.add_dependency 'rspec-core' |
Since bundler
is not so small (400KB+ since 2.4.3), I prefer not to include extra dependency.
I hope Ruby's native mechanism like defined?(Rails) && Rails.respond_to?(:application)
, defined?(Hanami) && Hanami.respond_to?(....)
can be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried not using bundler and using the standard methods you suggested, however at this point it seems due to the loading order, these constants don't exist yet.
If i try this:
module SharedHooks
def self.find_extractor
if defined?(Rails) && Rails.respond_to?(:application) && Rails.application
require 'rspec/openapi/extractors/rails'
RSpec::OpenAPI::Extractors::Rails
elsif defined?(Hanami) && Hanami.respond_to?(:app) && Hanami.app?
require 'rspec/openapi/extractors/hanami'
RSpec::OpenAPI::Extractors::Hanami
# elsif defined?(Roda)
# some Roda extractor
else
require 'rspec/openapi/extractors/rack'
RSpec::OpenAPI::Extractors::Rack
end
end
end
it won't work for hanami because of the way I use to throw the inspector into the router.
Would be glad for help in this aspect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
begin
require 'hanami'
require 'rspec/openapi/extractors/hanami'
rescue
puts 'Hanami not detected'
end
begin
require 'rails'
require 'rspec/openapi/extractors/rails'
rescue
puts 'Rails not detected'
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent solution
@@ -0,0 +1,13 @@ | |||
module SharedHooks |
Check notice
Code scanning / Rubocop
Document classes and non-namespace modules. Note
@@ -0,0 +1,13 @@ | |||
module SharedHooks |
Check notice
Code scanning / Rubocop
Add the frozen_string_literal comment to the top of files to help transition to frozen string literals by default. Note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks
This was released in v0.17.0 |
As part of this MR I added the ability to analyse the hanami router for proper documentation generation and to support generation of tags and summary similar to how it happens in rails.
I can also highlight the following changes:
Less significant changes:
Now, it seems that in almost all cases the extractors for hanami and rails work identically, but I could be mistaken.