Code review feedback

Author: neilvcarvalho

docs/src/sequences/setting-the-value.md

@@ -48,8 +48,8 @@ generate(:user, :with_email, :email) # "user_1234567@example.com"
 
 - A fixed collection sequence can accept any value within the collection.
 
-- An unlimited sequence, such as a charcter `sequence(:unlimited,'a')` will timeout if not found within the default maximum search time of three seconds.
+- An unlimited sequence, such as a character `sequence(:unlimited,'a')` will timeout if not found within the default maximum search time of three seconds.
 
-- The timeout can be set with: `ENV["FACTORY_BOT_SET_SEQUENCE_TIMEOUT"] = "1.5"`
+- The timeout can be configured with: `FactoryBot.sequence_setting_timeout = 1.5`
 
 </div>

lib/factory_bot.rb

@@ -59,6 +59,9 @@ module FactoryBot
   mattr_accessor :automatically_define_enum_traits, instance_accessor: false
   self.automatically_define_enum_traits = true
 
+  mattr_accessor :sequence_setting_timeout, instance_accessor: false
+  self.sequence_setting_timeout = 3
+
   # Look for errors in factories and (optionally) their traits.
   # Parameters:
   # factories - which factories to lint; omit for all factories
@@ -74,14 +77,38 @@ def self.lint(*args)
 
   # Set the starting value for ids when using the build_stubbed strategy
   #
-  # Arguments:
-  # * starting_id +Integer+
-  #   The new starting id value.
+  # @param [Integer] starting_id The new starting id value.
   def self.build_stubbed_starting_id=(starting_id)
     Strategy::Stub.next_id = starting_id - 1
   end
 
   class << self
+    # @!method rewind_sequence(*uri_parts)
+    #   Rewind an individual global or inline sequence.
+    #
+    #   @param [Array<Symbol>, String] uri_parts The components of the sequence URI.
+    #
+    #   @example Rewinding a sequence by its URI parts
+    #     rewind_sequence(:factory_name, :trait_name, :sequence_name)
+    #
+    #   @example Rewinding a sequence by its URI string
+    #     rewind_sequence("factory_name/trait_name/sequence_name")
+    #
+    # @!method set_sequence(*uri_parts, value)
+    #   Set the sequence to a specific value, providing the new value is within
+    #   the sequence set.
+    #
+    #   @param [Array<Symbol>, String] uri_parts The components of the sequence URI.
+    #   @param [Object] value The new value for the sequence. This must be a value that is
+    #     within the sequence definition. For example, you cannot set
+    #     a String sequence to an Integer value.
+    #
+    #   @example
+    #     set_sequence(:factory_name, :trait_name, :sequence_name, 450)
+    #   @example
+    #     set_sequence([:factory_name, :trait_name, :sequence_name], 450)
+    #   @example
+    #     set_sequence("factory_name/trait_name/sequence_name", 450)
     delegate :factories,
       :register_strategy,
       :rewind_sequences,

lib/factory_bot/definition.rb

@@ -1,12 +1,12 @@
 module FactoryBot
   # @api private
   class Definition
-    attr_reader :defined_traits, :declarations, :name, :registered_enums, :uri_mgr
+    attr_reader :defined_traits, :declarations, :name, :registered_enums, :uri_manager
     attr_accessor :klass
 
     def initialize(name, base_traits = [], **opts)
       @name = name
-      @uri_mgr = opts[:uri_mgr]
+      @uri_manager = opts[:uri_manager]
       @declarations = DeclarationList.new(name)
       @callbacks = []
       @defined_traits = Set.new

lib/factory_bot/definition_proxy.rb

@@ -121,7 +121,7 @@ def method_missing(name, *args, &block) # rubocop:disable Style/MissingRespondTo
     # Except that no globally available sequence will be defined.
     def sequence(name, *args, &block)
       options = args.extract_options!
-      options[:uri_paths] = definition.uri_mgr.to_a
+      options[:uri_paths] = definition.uri_manager.to_a
       args << options
 
       new_sequence = Sequence.new(name, *args, &block)
@@ -177,7 +177,7 @@ def factory(name, options = {}, &block)
     end
 
     def trait(name, &block)
-      @definition.define_trait(Trait.new(name, uri_paths: definition.uri_mgr.to_a, &block))
+      @definition.define_trait(Trait.new(name, uri_paths: definition.uri_manager.to_a, &block))
     end
 
     # Creates traits for enumerable values.
@@ -262,7 +262,7 @@ def __valid_association_options?(options)
     # return that one, otherwise register and return the given sequence
     #
     def __fetch_or_register_sequence(sequence)
-      FactoryBot::Sequence.find_by_uri(sequence.uri_mgr.first) ||
+      FactoryBot::Sequence.find_by_uri(sequence.uri_manager.first) ||
         FactoryBot::Internal.register_inline_sequence(sequence)
     end
   end

lib/factory_bot/factory.rb

@@ -12,8 +12,8 @@ def initialize(name, options = {})
       @parent = options[:parent]
       @aliases = options[:aliases] || []
       @class_name = options[:class]
-      @uri_mgr = FactoryBot::UriManager.new(names)
-      @definition = Definition.new(@name, options[:traits] || [], uri_mgr: @uri_mgr)
+      @uri_manager = FactoryBot::UriManager.new(names)
+      @definition = Definition.new(@name, options[:traits] || [], uri_manager: @uri_manager)
       @compiled = false
     end
 

lib/factory_bot/internal.rb

@@ -60,18 +60,6 @@ def rewind_sequences
         rewind_inline_sequences
       end
 
-      ##
-      # Rewind an individual global or inline sequence.
-      #
-      # Arguments:
-      #   uri_parts: (Array of Symbols)
-      #     The components of the sequence URI
-      #
-      # Example:
-      #  rewind_sequence(:factory_name, :trait_name, :sequence_name)
-      #          or
-      #  rewind_sequence("factory_name/trait_name/sequence_name")
-      #
       def rewind_sequence(*uri_parts)
         fail_argument_count(0, "1+") if uri_parts.empty?
 
@@ -81,25 +69,6 @@ def rewind_sequence(*uri_parts)
         sequence.rewind
       end
 
-      ##
-      # Set the squence to a specific value, providing the new value is
-      #   within the sequence set.
-      #
-      # Arguments:
-      #   uri_parts: (Array of Symbols)
-      #     The URI components of the sequence
-      #   value: (Object)
-      #     The new value for the sequence. This must be a value that is
-      #     within the sequence definition. For example, you cannot set
-      #     a String sequence to an Integer value.
-      #
-      # Example:
-      #  set_sequence(:factory_name, :trait_name, :sequence_name, 450)
-      #  or
-      #  set_sequence([:factory_name, :trait_name, :sequence_name], 450)
-      #  or
-      #  set_sequence("factory_name/trait_name/sequence_name", 450)
-      #
       def set_sequence(*uri_parts, value)
         uri = build_uri(uri_parts) || fail_argument_count(uri_parts.size, "2+")
         sequence = Sequence.find(*uri) || fail_unregistered_sequence(uri)
@@ -135,10 +104,6 @@ def register_default_strategies
         register_strategy(:null, FactoryBot::Strategy::Null)
       end
 
-      # ======================================================================
-      # = PRIVATE
-      # ======================================================================
-      #
       private
 
       def build_uri(...)

lib/factory_bot/sequence.rb

@@ -5,23 +5,8 @@ module FactoryBot
   # Sequence values are generated using next.
   # @api private
   class Sequence
-    attr_reader :name, :uri_mgr, :aliases
+    attr_reader :name, :uri_manager, :aliases
 
-    @sequence_setting_timeout = nil
-
-    # = Class Methods
-    # ======================================================================
-
-    ##
-    # Find a sequence by context and name.
-    #
-    # Arguments:
-    #   uri_parts: (Array of Strings or Symbols)
-    #             comprises the sequence URI parts
-    #
-    # Example:
-    #   Sequence.find :my_factory, :my_trait, :my_sequence
-    #
     def self.find(*uri_parts)
       if uri_parts.empty?
         fail ArgumentError, "wrong number of arguments, expected 1+)"
@@ -30,42 +15,18 @@ def self.find(*uri_parts)
       end
     end
 
-    ##
-    # Find a sequence by URI. Searches both Global and inline sequences.
-    #
-    # Arguments:
-    #   uri: (Symbol)
-    #     The uri name to search for
-    #
     def self.find_by_uri(uri)
       uri = uri.to_sym
       (FactoryBot::Internal.sequences.to_a.find { |seq| seq.has_uri?(uri) }) ||
         (FactoryBot::Internal.inline_sequences.find { |seq| seq.has_uri?(uri) })
     end
 
-    ##
-    # Returns the number of seconds to allow for setting sequence.
-    # Defaults to 3 seconds, but can be overriden with an
-    # environment variable 'FACTORY_BOT_SET_SEQUENCE_TIMEOUT'
-    #
-    def self.sequence_setting_timeout
-      @sequence_setting_timeout ||= begin
-        user_time = ENV["FACTORY_BOT_SET_SEQUENCE_TIMEOUT"].to_f
-        (user_time >= 0.1) ? user_time : 3
-      rescue
-        3
-      end
-    end
-
-    # = Instance Methods
-    # ======================================================================
-
     def initialize(name, *args, &proc)
       options = args.extract_options!
       @name = name
       @proc = proc
       @aliases = options.fetch(:aliases, []).map(&:to_sym)
-      @uri_mgr = FactoryBot::UriManager.new(names, paths: options[:uri_paths])
+      @uri_manager = FactoryBot::UriManager.new(names, paths: options[:uri_paths])
       @value = args.first || 1
 
       unless @value.respond_to?(:peek)
@@ -94,7 +55,7 @@ def has_name?(test_name)
     end
 
     def has_uri?(uri)
-      uri_mgr.include?(uri)
+      uri_manager.include?(uri)
     end
 
     def for_factory?(test_factory_name)
@@ -109,13 +70,13 @@ def rewind
     # If it's an Integer based sequence, set the new value directly,
     # else rewind and seek from the beginning until a match is found.
     #
-    def set_value(new_val)
-      if can_set_value_directly?(new_val)
-        @value.set_value(new_val)
+    def set_value(new_value)
+      if can_set_value_directly?(new_value)
+        @value.set_value(new_value)
       elsif can_set_value_by_index?
-        set_value_by_index(new_val)
+        set_value_by_index(new_value)
       else
-        seek_value(new_val)
+        seek_value(new_value)
       end
     end
 
@@ -138,10 +99,10 @@ def can_set_value_by_index?
     end
 
     ##
-    # Set to the given val, or fail if not found
+    # Set to the given value, or fail if not found
     #
-    def set_value_by_index(val)
-      index = @value.find_index(val) || fail_value_not_found(val)
+    def set_value_by_index(value)
+      index = @value.find_index(value) || fail_value_not_found(value)
       @value.rewind
       index.times { @value.next }
     end
@@ -150,14 +111,14 @@ def set_value_by_index(val)
     # Rewind index and seek until the value is found or the max attempts
     # have been tried. If not found, the sequence is rewound to its original value
     #
-    def seek_value(val)
+    def seek_value(value)
       original_value = @value.peek
 
       # rewind and search for the new value
       @value.rewind
-      Timeout.timeout(Sequence.sequence_setting_timeout) do
+      Timeout.timeout(FactoryBot.sequence_setting_timeout) do
         loop do
-          return if @value.peek == val
+          return if @value.peek == value
           increment_value
         end
 
@@ -166,23 +127,26 @@ def seek_value(val)
         fail StopIteration
       end
     rescue Timeout::Error, StopIteration
-      reset_originl_value(original_value)
-      fail_value_not_found(val)
+      reset_original_value(original_value)
+      fail_value_not_found(value)
     end
 
-    def reset_originl_value(original_value)
+    def reset_original_value(original_value)
       @value.rewind
-      until @value.peek == original_value do increment_value end
+
+      until @value.peek == original_value
+        increment_value
+      end
     end
 
-    def can_set_value_directly?(val)
-      return false unless val.is_a?(Integer)
+    def can_set_value_directly?(value)
+      return false unless value.is_a?(Integer)
       return false unless @value.is_a?(EnumeratorAdapter)
       @value.integer_value?
     end
 
-    def fail_value_not_found(val)
-      fail ArgumentError, "Unable to find '#{val}' in the sequence."
+    def fail_value_not_found(value)
+      fail ArgumentError, "Unable to find '#{value}' in the sequence."
     end
 
     class EnumeratorAdapter
@@ -203,9 +167,9 @@ def rewind
         @value = @first_value
       end
 
-      def set_value(new_val)
-        if new_val >= @first_value
-          @value = new_val
+      def set_value(new_value)
+        if new_value >= @first_value
+          @value = new_value
         else
           fail ArgumentError, "Value cannot be less than: #{@first_value}"
         end

lib/factory_bot/trait.rb

@@ -9,9 +9,9 @@ class Trait
     def initialize(name, **options, &block)
       @name = name.to_s
       @block = block
-      @uri_mgr = FactoryBot::UriManager.new(names, paths: options[:uri_paths])
+      @uri_manager = FactoryBot::UriManager.new(names, paths: options[:uri_paths])
 
-      @definition = Definition.new(@name, uri_mgr: @uri_mgr)
+      @definition = Definition.new(@name, uri_manager: @uri_manager)
       proxy = FactoryBot::DefinitionProxy.new(@definition)
 
       if block
@@ -20,7 +20,7 @@ def initialize(name, **options, &block)
     end
 
     def clone
-      Trait.new(name, uri_paths: definition.uri_mgr.paths, &block)
+      Trait.new(name, uri_paths: definition.uri_manager.paths, &block)
     end
 
     def names