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

Add config.mission_control.jobs.irb_help that defaults to true #84

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

dorianmariecom
Copy link
Contributor

To display or hide the irb help message

Fixes #76

@@ -11,7 +11,7 @@ class Engine < ::Rails::Engine
isolate_namespace MissionControl::Jobs

config.mission_control = ActiveSupport::OrderedOptions.new unless config.try(:mission_control)
config.mission_control.jobs = ActiveSupport::OrderedOptions.new
config.mission_control.jobs = ActiveSupport::OrderedOptions.new(irb_help: true)
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to add this single option here? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to provide a default, otherwise it's nil, I don't get why the default: true doesn't work

Copy link
Member

Choose a reason for hiding this comment

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

That's very odd, you should dig into that instead of just patching it like this, without understanding what's happening.

@@ -16,5 +16,6 @@ module Jobs
mattr_accessor :delay_between_bulk_operation_batches, default: 0
mattr_accessor :logger, default: ActiveSupport::Logger.new(nil)
mattr_accessor :internal_query_count_limit, default: 500_000 # Hard limit to keep unlimited count queries fast enough
mattr_accessor :irb_help, default: true
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we could call this show_console_help or something like this, to be a bit more explicit? We use console in Rails initialization, rather than irb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done

@dorianmariecom dorianmariecom force-pushed the dorian/irb-help branch 2 times, most recently from 1ec92a2 to 7e4dd1e Compare February 23, 2024 07:10
@@ -11,17 +11,15 @@ class Engine < ::Rails::Engine
isolate_namespace MissionControl::Jobs

config.mission_control = ActiveSupport::OrderedOptions.new unless config.try(:mission_control)
config.mission_control.jobs = ActiveSupport::OrderedOptions.new
config.mission_control.jobs = MissionControl::Jobs
Copy link
Member

Choose a reason for hiding this comment

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

We don't want the config to point to this module, we want mission_control.jobs to behave as any other app config. This should be kept as it was, as ActiveSupport::OrderedOptions.new.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, done

@@ -79,7 +77,9 @@ class Engine < ::Rails::Engine
MissionControl::Jobs::Current.server = application.servers.first
end

puts "\n\nType 'jobs_help' to see how to connect to the available job servers to manage jobs\n\n"
if config.mission_control.jobs.show_console_help
Copy link
Member

Choose a reason for hiding this comment

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

I think you should use MissionControl::Jobs.show_console_help here, as the initialization process has completed if you're opening a console.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that works, done and rebased

Copy link
Member

@rosa rosa left a comment

Choose a reason for hiding this comment

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

Perfect, thank you so much! 🙏

@rosa rosa merged commit a82578a into rails:main Mar 1, 2024
5 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.

How to remove the jobs_help messsage from irb?
2 participants