Skip to content

Conversation

@ofedoren
Copy link
Member

@ekohl, you might be interested in this, since it's basically an alternative to Apipie/apipie-rails#946. I don't even know myself how we didn't try this earlier :/ The new validator probably could be re-used in Katello or wherever similar places we have.

Before:
Screenshot-1742990986331

After:
Screenshot-1742990922256

@ofedoren ofedoren force-pushed the bug-38315-missing-status-types branch from 6d0040f to 97f785f Compare March 26, 2025 12:16
@ekohl
Copy link
Member

ekohl commented Mar 28, 2025

I likevthe idea but do you intend to submit it to apipie or keep it in here?

@ofedoren
Copy link
Member Author

do you intend to submit it to apipie or keep it in here?

I think it's better to live here, since it's quite custom. Not sure how could I generalize this to submit to Apipie-rails.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I considered wrapping the block in a specific class so you could distinguish between a regular callable and a callable that's an enum. But I can see your argument that it's rather specific and going to be messy so I won't object to merging it here.

api :GET, "/hosts/:id/status/:type", N_("Get status of host")
param :id, :identifier_dottable, :required => true
param :type, [HostStatus::Global] + HostStatus.status_registry.to_a.map { |s| s.humanized_name }, :required => true, :desc => N_(
param :type, :delayed_enum, of: -> { [HostStatus::Global.status_name.underscore] + HostStatus.status_registry.to_a.map { |s| s.humanized_name } }, required: true, desc: N_(
Copy link
Member

Choose a reason for hiding this comment

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

The desc is now autogenerated so can we drop this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess so, dropped. I think the only benefit of such description is that it should be translated.

api :DELETE, "/hosts/:id/status/:type", N_("Clear sub-status of host")
param :id, :identifier_dottable, :required => true
param :type, HostStatus.status_registry.to_a.map { |s| s.humanized_name }, :required => true, :desc => N_(
param :type, :delayed_enum, of: -> { HostStatus.status_registry.to_a.map { |s| s.humanized_name } }, required: true, desc: N_(
Copy link
Member

Choose a reason for hiding this comment

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

Same here: can we drop the description and rely on the generated one?

end
end

class DelayedEnumValidator < Apipie::Validator::BaseValidator
Copy link
Member

Choose a reason for hiding this comment

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

Bike shedding: CallableEnumValidator?

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed

@ofedoren ofedoren force-pushed the bug-38315-missing-status-types branch from 97f785f to 9eb6ef2 Compare April 1, 2025 14:47
Copy link
Member Author

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @ekohl, updated!

end
end

class DelayedEnumValidator < Apipie::Validator::BaseValidator
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed

api :GET, "/hosts/:id/status/:type", N_("Get status of host")
param :id, :identifier_dottable, :required => true
param :type, [HostStatus::Global] + HostStatus.status_registry.to_a.map { |s| s.humanized_name }, :required => true, :desc => N_(
param :type, :delayed_enum, of: -> { [HostStatus::Global.status_name.underscore] + HostStatus.status_registry.to_a.map { |s| s.humanized_name } }, required: true, desc: N_(
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess so, dropped. I think the only benefit of such description is that it should be translated.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

On my phone now so can't verify it but code reads well

@ekohl ekohl merged commit 629c796 into theforeman:develop Apr 8, 2025
31 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.

2 participants