-
Notifications
You must be signed in to change notification settings - Fork 991
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 #36575 - Use apipie-dsl default model descriptions #9867
Conversation
86592e3
to
0a58cde
Compare
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'd squash this in a single commit, but in the same commit also remove all places where the model description is currently set. Otherwise the default will never be used. Then the summary of the commit would be to something like Use apipie-dsl default model descriptions
.
0a58cde
to
ed612ee
Compare
ed612ee
to
3e4d60e
Compare
d2b3052
to
45cb2ba
Compare
45cb2ba
to
b7082e7
Compare
0449f58
to
46af197
Compare
46af197
to
9ccdc66
Compare
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.
This is indeed roughly what I would expect. There are 2 more cases I see:
$ rg 'A class represent'
config/initializers/apipie.rb
30: config.default_class_description = ->(model) { _("A class representing %s object") % model.human }
app/models/hostgroup.rb
86: apipie :class, "A class representing #{model_name.human} object" do
app/models/subnet.rb
127: apipie :class, "A class representing #{model_name.human} object" do
app/models/ptable.rb
64: apipie :class, desc: 'A class representing Partition Table object' do
app/models/nic/base.rb
83: apipie :class, 'A class representing Network Interface Controller object' do
app/models/host/base.rb
80: apipie :class, 'A class representing base Host object' do
app/models/host/managed.rb
106: apipie :class, 'A class representing managed Host object' do
So app/models/hostgroup.rb
and app/models/subnet.rb
are also candidates.
9ccdc66
to
4529ebb
Compare
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.
Test failure looks related:
[2023-10-20T09:35:11.463Z] NoMethodError: undefined method `human' for User (call 'User.connection' to establish a connection):Class
[2023-10-20T09:35:11.463Z] /usr/local/rvm/gems/ruby-2.7.4@foreman-pr-unit-test-1661-2/gems/activerecord-6.1.7.6/lib/active_record/dynamic_matchers.rb:22:in `method_missing'
4529ebb
to
60e921d
Compare
@ekohl I'm not sure where you're pointing it to, Can you please explain? |
I opened the unit test results. The current one fails with another error (https://ci.theforeman.org/blue/organizations/jenkins/foreman-pr-unit-test/detail/foreman-pr-unit-test/1672/pipeline/) but I also see this in the Katello failure:
|
60e921d
to
5c6a156
Compare
And we're back to this error:
@ofedoren I think that it calls the descriptions during the initializer is a problem and a blocker to using this feature. Do you agree? @archanaserver perhaps you can look in apipie-dsl itself to see if this can be fixed. |
@ekohl @ofedoren I didn't find anything related to this issue. Could you please point me in the right direction? I'll take another look. |
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've released apipie-dsl 2.6.1 with a fix to postpone default description block evaluation until the actual description renders. It should help for most of the time.
Although, there should be a slight change in config here. Please see inline.
config/initializers/apipie.rb
Outdated
@@ -27,6 +27,8 @@ | |||
trans | |||
end | |||
config.help_layout = 'apipie_dsl/apipie_dsls/help.html.erb' | |||
config.default_class_description = ->(model) { _("A class representing %s object") % model.human } |
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.
config.default_class_description = ->(model) { _("A class representing %s object") % model.human } | |
config.default_class_description = ->(model) do | |
return nil unless model.respond_to?(:model_name) | |
_("A class representing %s object") % model.model_name.human | |
end |
It appears that model
doesn't necessary respond to human
, but rather should respond (in case it's ActiveRecord
) to model_name
, which returns some wrapper, which responds to human
.
5c6a156
to
1be553b
Compare
Gemfile
Outdated
@@ -11,7 +11,7 @@ gem 'ancestry', '~> 4.0' | |||
gem 'scoped_search', '>= 4.1.10', '< 5' | |||
gem 'ldap_fluff', '>= 0.5.0', '< 1.0' | |||
gem 'apipie-rails', '>= 0.8.0', '< 2' | |||
gem 'apipie-dsl', '>= 2.2.6' | |||
gem 'apipie-dsl', '>= 2.6.0' |
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.
This should at least require 2.6.1
gem 'apipie-dsl', '>= 2.6.0' | |
gem 'apipie-dsl', '>= 2.6.1' |
config/initializers/apipie.rb
Outdated
@@ -27,6 +27,11 @@ | |||
trans | |||
end | |||
config.help_layout = 'apipie_dsl/apipie_dsls/help.html.erb' | |||
config.default_class_description = ->(model) do |
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 think RuboCop wants to use lambda:
config.default_class_description = ->(model) do | |
config.default_class_description = lambda do |model| |
See https://docs.rubocop.org/rubocop/1.57/cops_style.html#stylelambda as well
1be553b
to
24c075f
Compare
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.
Packit fails to build because it wasn't rebased, but that's not blocking here.
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.
Nothing to add from my side, thanks a lot! Let's get this in.
This pull request updates the
apipie-dsl
gem to version2.6.1
and makes configuration changes to enhance our API documentation.