From 80aa8a77eb66e54ef8eb019cc17629baaf1d9f89 Mon Sep 17 00:00:00 2001 From: Sylvain Joyeux Date: Wed, 23 May 2018 11:30:05 -0300 Subject: [PATCH] style and warning fixes --- .../models/action_state_machine.rb | 8 ++--- lib/roby/coordination/task_script.rb | 9 ++---- lib/roby/execution_engine.rb | 7 ++--- lib/roby/plan.rb | 21 +++++++------- lib/roby/task_structure/dependency.rb | 17 +++++------ .../models/test_action_state_machine.rb | 13 ++++----- test/coordination/test_action_script.rb | 7 ++--- .../coordination/test_action_state_machine.rb | 6 ++-- test/coordination/test_task_script.rb | 3 +- test/test_execution_engine.rb | 29 +++++++++++-------- 10 files changed, 56 insertions(+), 64 deletions(-) diff --git a/lib/roby/coordination/models/action_state_machine.rb b/lib/roby/coordination/models/action_state_machine.rb index 11e3a045d..6bd82b664 100644 --- a/lib/roby/coordination/models/action_state_machine.rb +++ b/lib/roby/coordination/models/action_state_machine.rb @@ -36,9 +36,9 @@ module Models # stand = state move(speed: 0) # # This monitor triggers each time the system moves more than # # 0.1 meters - # d_monitor = task monitor_movement_threshold(d: 0.1) + # d_monitor = task monitor_movement_threshold(d: 0.1) # # This monitor triggers after 20 seconds - # t_monitor = task monitor_time_threshold(t: 20) + # t_monitor = task monitor_time_threshold(t: 20) # # Make the distance monitor run in the move state # move.depends_on d_monitor # # Make the time monitor run in the stand state @@ -127,7 +127,7 @@ def capture(state, event = nil, &block) if block lambda(&block) else - lambda { |event| event.context.first } + lambda { |ev| ev.context.first } end capture = Capture.new(filter) @@ -224,5 +224,3 @@ def to_s end end end - - diff --git a/lib/roby/coordination/task_script.rb b/lib/roby/coordination/task_script.rb index 6145b07c1..e44531506 100644 --- a/lib/roby/coordination/task_script.rb +++ b/lib/roby/coordination/task_script.rb @@ -88,7 +88,7 @@ def resolve_event(event) raise ArgumentError, "#{model.root} has no event called #{symbol}" end end - script_task, model_task = resolve_task(event.task) + _, model_task = resolve_task(event.task) model_event = model_task.find_event(event.symbol) script_event = instance_for(model_event) return script_event, model_event @@ -191,7 +191,7 @@ def transition! # emit an event (and quit the block) # # @param [Hash] options - # @option options [Event] :event (nil) if set, the given event will + # @option options [Event] :emit (nil) if set, the given event will # be emitted when the timeout is reached. Otherwise, a # Script::TimedOut exception is generated with the script's # supporting task as origin @@ -207,7 +207,7 @@ def timeout(seconds, options = Hash.new, &block) def timeout_start(seconds, options = Hash.new) options, timeout_options = Kernel.filter_options options, emit: nil if event = options[:emit] - script_event, model_event = resolve_event(event) + _, model_event = resolve_event(event) end model.timeout_start(seconds, timeout_options.merge(emit: model_event)) end @@ -288,6 +288,3 @@ def transition! end end end - - - diff --git a/lib/roby/execution_engine.rb b/lib/roby/execution_engine.rb index 3dd41218c..1ec48eccc 100644 --- a/lib/roby/execution_engine.rb +++ b/lib/roby/execution_engine.rb @@ -559,7 +559,6 @@ def gather_propagation(initial_set = Hash.new) @propagation_sources = nil @propagation_step_id = 0 - before = @propagation propagation_context([]) do yield end @@ -855,7 +854,7 @@ def error_handling_phase(events_errors) # Compute the set of unhandled fatal exceptions def compute_kill_tasks_for_unhandled_fatal_errors(fatal_errors) - kill_tasks = fatal_errors.inject(Set.new) do |tasks, (exception, affected_tasks)| + kill_tasks = fatal_errors.inject(Set.new) do |tasks, (_exception, affected_tasks)| tasks.merge(affected_tasks) end # Tasks might have been finalized during exception handling, filter @@ -1052,7 +1051,7 @@ def prepare_propagation(target, is_forward, info) # the forwardings and signals that the propagation of the considered event # have added. def event_propagation_step(current_step, propagation_info) - signalled, step_id, forward_info, call_info = next_event(current_step) + signalled, _step_id, forward_info, call_info = next_event(current_step) next_step = nil if !call_info.empty? @@ -2440,7 +2439,7 @@ def execute(catch: [], type: :external_events) when :ret return value when :throw - throw *value + throw(*value) else raise value end diff --git a/lib/roby/plan.rb b/lib/roby/plan.rb index 645c1ff85..71d5f3d90 100644 --- a/lib/roby/plan.rb +++ b/lib/roby/plan.rb @@ -4,7 +4,7 @@ class Plan < DistributedObject extend Logger::Hierarchy extend Logger::Forward include DRoby::EventLogging - + # The Peer ID of the local owner (i.e. of the local process / execution # engine) attr_accessor :local_owner @@ -219,7 +219,7 @@ def merge_base(plan) task_index.merge(plan.task_index) task_events.merge(plan.task_events) end - + def merge_relation_graphs(plan) # Now merge the relation graphs # @@ -607,7 +607,7 @@ def add_permanent_event(event) # True if the given event is registered as a permanent event on self def permanent_event?(generator) - @permanent_events.include?(generator) + @permanent_events.include?(generator) end # Removes a task from the set of permanent tasks @@ -677,7 +677,7 @@ def handle_force_replace(from, to) elsif to.plan != self raise ArgumentError, "trying to replace #{to} but its plan is #{to.plan}, expected #{self}" elsif from == to - return + return end # Swap the subplans of +from+ and +to+ @@ -758,7 +758,7 @@ def exclude_tasks(tasks) @tasks.merge(tasks) self end - + # Exclude a single task # # @param [Task] task the task to be excluded @@ -767,7 +767,7 @@ def exclude_task(task) @tasks << task self end - + # Tests whether a task is to be excluded # # @param [Task] task @@ -1378,7 +1378,7 @@ def each_task return enum_for(__method__) if !block_given? @tasks.each { |t| yield(t) } end - + # Returns +object+ if object is a plan object from this plan, or if # it has no plan yet (in which case it is added to the plan first). # Otherwise, raises ArgumentError. @@ -1592,7 +1592,7 @@ def replan(task) return task.create_fresh_copy end - planner = replan(old_planner = task.planning_task) + planner = replan(task.planning_task) planned = task.create_fresh_copy planned.abstract = true planned.planned_by planner @@ -1600,7 +1600,7 @@ def replan(task) planned end - + # The set of blocks that should be called to check the structure of the # plan. # @@ -1635,7 +1635,7 @@ def self.check_failed_missions(plan) result end structure_checks << method(:check_failed_missions) - + # @api private # # Normalize the value returned by one of the {#structure_checks}, by @@ -1909,4 +1909,3 @@ def each_object_in_transaction_stack(object) end end end - diff --git a/lib/roby/task_structure/dependency.rb b/lib/roby/task_structure/dependency.rb index db15e15c2..829e0a402 100644 --- a/lib/roby/task_structure/dependency.rb +++ b/lib/roby/task_structure/dependency.rb @@ -69,7 +69,7 @@ def self.merge_fullfilled_model(model, required_models, required_arguments) end end - arguments = arguments.merge(required_arguments) do |name, old, new| + arguments = arguments.merge(required_arguments) do |name, old, new| if old != new raise Roby::ModelViolation, "inconsistency in fullfilled models: #{old} and #{new}" end @@ -434,7 +434,7 @@ def resolve_role_path(*path) # +self+. # # I.e. if ['role1', 'role2', 'role3'] is a role path from +self+ to - # +task, it means that + # +task, it means that # # task1 = self.child_from_role('role1') # task2 = task1.child_from_role('role2') @@ -479,7 +479,7 @@ def role_paths(task, validate = true) # # success:: the list of success events. The default is [:success] # failure:: the list of failing events. The default is [:failed] - # model:: + # model:: # a [task_model, arguments] pair which defines the task # model the parent is expecting. The default value is to get these # parameters from +task+ @@ -504,9 +504,9 @@ def depends_on(task, options = {}) raise ArgumentError, "cannot add a dependency of a task to itself" end - options = Dependency.validate_options options, - model: [task.provided_models, task.meaningful_arguments], - success: :success.to_unbound_task_predicate, + options = Dependency.validate_options options, + model: [task.provided_models, task.meaningful_arguments], + success: :success.to_unbound_task_predicate, failure: false.to_unbound_task_predicate, remove_when_done: true, consider_in_pending: true, @@ -639,7 +639,7 @@ def first_children # However, this fails in case +self+ is a root task in the dependency # relation. Moreover, it might be handy to over-constrain the model # computed through the dependency relation. - # + # # In both cases, a model can be specified explicitely by setting the # fullfilled_model attribute. The value has to be # @@ -826,7 +826,7 @@ def implicit_fullfilled_model def fullfilled_model explicit_fullfilled_model || implicit_fullfilled_model end - + # Enumerates the models that all instances of this task model fullfill # # @yieldparam [Model,Model] model @@ -929,4 +929,3 @@ def involved_plan_object?(obj) end end end - diff --git a/test/coordination/models/test_action_state_machine.rb b/test/coordination/models/test_action_state_machine.rb index 6b64a9e32..05592590f 100644 --- a/test/coordination/models/test_action_state_machine.rb +++ b/test/coordination/models/test_action_state_machine.rb @@ -152,7 +152,7 @@ def start_task(task) it "raises if attempting to specify a state that is not a toplevel state" do monitoring = state_machine.state(action_m.monitoring_task) start.depends_on(monitoring) - + assert_raises(Roby::Coordination::Models::NotToplevelState) do state_machine.forward monitoring, monitoring.success_event, state_machine.success_event @@ -161,7 +161,7 @@ def start_task(task) it "raises if attempting to specify an event that is not active in the state" do monitoring = state_machine.state(action_m.monitoring_task) state_machine.transition start.success_event, monitoring - + assert_raises(Roby::Coordination::Models::EventNotActiveInState) do state_machine.forward start, monitoring.success_event, state_machine.success_event @@ -206,7 +206,7 @@ def start_task(task) describe "#state" do it "can use any object that responds to #to_action_state" do obj = flexmock - obj.should_receive(:to_action_state).and_return(task = Roby::Task.new) + obj.should_receive(:to_action_state).and_return(Roby::Task.new) task_m = self.task_m assert_raises(ArgumentError) do Roby::Actions::Interface.new_submodel do @@ -292,10 +292,9 @@ def start_task(task) coordination_model assert_equal 1, rebound.forwards.size - source_event, target_event = rebound.forwards.first - assert_equal [[new_action_m.monitoring_task, 'task_dependency']], - rebound.find_state_by_name('start_task').dependencies.map { |task, role| [task.action, role] } + rebound.find_state_by_name('start_task').dependencies. + map { |task, role| [task.action, role] } end end @@ -352,7 +351,7 @@ def start_task(task) # task model can be transferred afterwards test_task_m = Roby::Task.new_submodel start_task_m = Roby::Task.new_submodel - description = action_m.describe("with_arguments"). + action_m.describe("with_arguments"). optional_arg('test', 'test', test_task_m) action_m.action_state_machine 'with_arguments' do start state(start_task_m) diff --git a/test/coordination/test_action_script.rb b/test/coordination/test_action_script.rb index 1ec873cd3..17953de35 100644 --- a/test/coordination/test_action_script.rb +++ b/test/coordination/test_action_script.rb @@ -50,7 +50,7 @@ def start_script end it "fails if asked to wait for an unreachable event" do - script = start_script + start_script expect_execution { action_task.intermediate_event.unreachable! }. to { have_error_matching Models::Script::DeadInstruction.match.with_origin(root_task) } end @@ -58,7 +58,7 @@ def start_script it "fails if the event it is waiting for becomes unreachable" do task_model.event :second script_model.wait script_task.second_event - script = start_script + start_script execute { action_task.second_event.unreachable! } expect_execution { action_task.intermediate_event.unreachable! }. to { have_error_matching Models::Script::DeadInstruction.match.with_origin(root_task) } @@ -135,7 +135,7 @@ def start_script and_return(action_task = task_model.new) script_model.forward script_task.intermediate_event, script_model.success_event script_model.start script_task - script = script_model.new(root_task) + script_model.new(root_task) execute { root_task.start! } expect_execution do action_task.start! @@ -148,4 +148,3 @@ def start_script end end end - diff --git a/test/coordination/test_action_state_machine.rb b/test/coordination/test_action_state_machine.rb index d839d04e9..797ed3b92 100644 --- a/test/coordination/test_action_state_machine.rb +++ b/test/coordination/test_action_state_machine.rb @@ -283,7 +283,7 @@ def start_task(task) state_machine = task.each_coordination_object.first flexmock(state_machine).should_receive(:instanciate_state_transition). and_raise(error_m = Class.new(Exception)) - + expect_execution do task.current_task_child.start! task.current_task_child.stop_event.emit @@ -343,8 +343,6 @@ def start_task(task) end it "can be passed actual state models as arguments" do - task_m = self.task_m - description.required_arg(:first_task, 'the first state') state_machine('test') do first_state = state(first_task) @@ -568,7 +566,7 @@ def test_it_can_pass_arguments_to_the_associated_fault_response_tables forward task.success_event, success_event end plan.add(task = task_m.new) - state_machine = state_machine_m.new(task) + state_machine_m.new(task) execute { task.start! } execute do diff --git a/test/coordination/test_task_script.rb b/test/coordination/test_task_script.rb index c0fd23556..776c26845 100644 --- a/test/coordination/test_task_script.rb +++ b/test/coordination/test_task_script.rb @@ -108,7 +108,6 @@ def self.event_source_as_child_behaviour(context) it "adds the child as a dependency" do event_source_task = self.event_source_task - recorder = flexmock root.script do wait event_source_task.start_event end @@ -497,7 +496,7 @@ def test_script_is_prepared_with_the_new_task_after_a_replacement model = Roby::Task.new_submodel { terminates } old, new = prepare_plan add: 2, model: model old.abstract = true - script = old.script do + old.script do emit success_event end plan.replace_task(old, new) diff --git a/test/test_execution_engine.rb b/test/test_execution_engine.rb index 31cc3d647..c0714f95b 100644 --- a/test/test_execution_engine.rb +++ b/test/test_execution_engine.rb @@ -155,7 +155,7 @@ module Roby describe "#propagation_context" do it "sets the sources to the given set" do execution_engine.gather_propagation do - execution_engine.propagation_context(sources = [event = flexmock]) do + execution_engine.propagation_context([event = flexmock]) do assert_equal [event], execution_engine.propagation_sources end end @@ -175,7 +175,7 @@ module Roby execution_engine.gather_propagation do execution_engine.propagation_context(original_sources = [flexmock]) do begin - execution_engine.propagation_context(sources = [flexmock]) do + execution_engine.propagation_context([flexmock]) do raise end ensure @@ -772,11 +772,14 @@ def match_exception(*edges, handled: nil) unhandled: [full_trace, Set[root, child]], handled: []) end - assert_match /some parents specified for.*are actually not parents of #{Regexp.quote(child.to_s)}, they got filtered out/, messages[0] + rx = Regexp.new("some parents specified for.*are actually not parents "\ + "of #{Regexp.quote(child.to_s)}, they got filtered out") + assert_match rx, messages[0] assert_equal " #{task}", messages[1] end - it "will propagate through all parents if filtering out non-existing parents results in an empty set" do + it "will propagate through all parents if filtering out "\ + "non-existing parents results in an empty set" do plan.add(task = task_m.new(name: 'task')) exception = localized_error_m.new(child).to_execution_exception @@ -794,7 +797,9 @@ def match_exception(*edges, handled: nil) unhandled: [full_trace, Set[root, child]], handled: []) end - assert_match /some parents specified for.*are actually not parents of #{Regexp.quote(child.to_s)}, they got filtered out/, messages[0] + rx = Regexp.new("some parents specified for.*are actually not parents "\ + "of #{Regexp.quote(child.to_s)}, they got filtered out") + assert_match rx, messages[0] assert_equal " #{task}", messages[1] end @@ -1223,7 +1228,7 @@ def assert_receives_notification(notification_type) mock_compute_errors :fatal_errors Roby.app.filter_backtraces = false assert_receives_notification ExecutionEngine::EXCEPTION_FATAL - messages = capture_log(execution_engine, :warn) do + capture_log(execution_engine, :warn) do execution_engine.process_events end end @@ -1732,7 +1737,7 @@ def wait_until_in_thread(generator) it "passes an exception raised within the block to the thread" do error = Class.new(RuntimeError) - thread = wait_until_in_thread task.start_event do + wait_until_in_thread task.start_event do raise error end execute { task.start! } @@ -2097,10 +2102,10 @@ def test_next_step plan.add [e1, e2, e3] pending = Array.new - def pending.each_key; each { |(k, v)| yield(k) } end + def pending.each_key; each { |(k, _)| yield(k) } end def pending.delete(ev) - value = find { |(k, v)| k == ev }.last - delete_if { |(k, v)| k == ev } + value = find { |(k, _)| k == ev }.last + delete_if { |(k, _)| k == ev } value end @@ -2125,7 +2130,7 @@ def pending.delete(ev) end def test_delayed_signal - Timecop.freeze(base_time = Time.now) + Timecop.freeze(Time.now) plan.add_mission_task(t = Tasks::Simple.new) e = EventGenerator.new(true) @@ -2370,7 +2375,7 @@ def test_stats sorted_by_time = timepoints.sort_by { |name, d| d } sorted_by_name = timepoints.sort_by { |name, d| time_events.index(name) } - sorted_by_time.each_with_index do |(name, d), i| + sorted_by_time.each_with_index do |(_name, d), i| assert(sorted_by_name[i][1] == d) end end