Merge pull request #141 from shangsuru/refactoring-and-linting

Refactoring

app/controllers/scimitar/application_controller.rb

@@ -47,7 +47,7 @@ def handle_resource_not_found(exception)
       #
       # *exception+:: If a Ruby exception was the reason this method is being
       #               called, pass it here. Any configured exception reporting
-      #               mechanism will be invokved with the given parameter.
+      #               mechanism will be invoked with the given parameter.
       #               Otherwise, the +error_response+ value is reported.
       #
       def handle_scim_error(error_response, exception = error_response)

app/controllers/scimitar/schemas_controller.rb

@@ -201,7 +201,7 @@ def redraw_schema_using_mappings(
             if mapped_multivalue_attribute.is_a?(Array)
 
               # A single-entry array with "list using" semantics, for a
-              # collection of an artbirary number of same-class items - e.g.
+              # collection of an arbitrary number of same-class items - e.g.
               # Groups to which a User belongs.
               #
               # If this is an up-to-date mapping, there's a "class" entry that

app/models/scimitar/lists/query_parser.rb

@@ -342,14 +342,14 @@ def flatten_filter(filter)
           skip_next_component       = false
 
           components.each.with_index do | component, index |
-            if skip_next_component == true
+            if skip_next_component
               skip_next_component = false
               next
             end
 
             downcased = component.downcase.strip
 
-            if (expecting_attribute)
+            if expecting_attribute
               if downcased.match?(/[^\\]\[/) # Not backslash then literal '['
                 attribute_prefix       = component.match(/(.*?[^\\])\[/   )[1] # Everything before no-backslash-then-literal (unescaped) '['
                 first_attribute_inside = component.match(    /[^\\]\[(.*)/)[1] # Everything  after no-backslash-then-literal (unescaped) '['
@@ -362,7 +362,7 @@ def flatten_filter(filter)
               expecting_attribute = false
               expecting_operator  = true
 
-            elsif (expecting_operator)
+            elsif expecting_operator
               rewritten << component
               if BINARY_OPERATORS.include?(downcased)
                 expecting_operator = false
@@ -374,7 +374,7 @@ def flatten_filter(filter)
                 raise 'Expected operator'
               end
 
-            elsif (expecting_value)
+            elsif expecting_value
               matches = downcased.match(/([^\\])\](.*)/) # Contains no-backslash-then-literal (unescaped) ']'; also capture anything after
               unless matches.nil? # Contains no-backslash-then-literal (unescaped) ']'
                 character_before_closing_bracket = matches[1]
@@ -395,7 +395,7 @@ def flatten_filter(filter)
                 # So - NOTE RECURSION AND EARLY EXIT POSSIBILITY HEREIN.
                 #
                 if (
-                  ! attribute_prefix.nil? &&
+                  !attribute_prefix.nil? &&
                   OPERATORS.key?(components[index + 1]&.downcase) &&
                   characters_after_closing_bracket.match?(/^\.#{ATTRNAME}$/)
                 )
@@ -437,7 +437,7 @@ def flatten_filter(filter)
               if downcased.start_with?('"')
                 expecting_closing_quote = true
                 downcased = downcased[1..-1] # Strip off opening '"' to avoid false-positive on 'contains closing quote' check below
-              elsif expecting_closing_quote == false # If not expecting a closing quote, then the component must be the entire no-spaces value
+              elsif !expecting_closing_quote # If not expecting a closing quote, then the component must be the entire no-spaces value
                 expecting_value      = false
                 expecting_logic_word = true
               end
@@ -450,7 +450,7 @@ def flatten_filter(filter)
                 end
               end
 
-            elsif (expecting_logic_word)
+            elsif expecting_logic_word
               if downcased == 'and' || downcased == 'or'
                 rewritten << component
                 next_downcased_component = components[index + 1].downcase.strip
@@ -586,11 +586,11 @@ def assert_eos
 
         # Recursively process an expression tree. Calls itself with nested tree
         # fragments. Each inner expression fragment calculates on the given
-        # base scope, with aggregration at each level into a wider query using
+        # base scope, with aggregation at each level into a wider query using
         # AND or OR depending on the expression tree contents.
         #
         # +base_scope+::      Base scope (ActiveRecord::Relation, e.g. User.all
-        #                     - neverchanges during recursion).
+        #                     - never changes during recursion).
         #
         # +expression_tree+:: Top-level expression tree or fragments inside if
         #                     self-calling recursively.
@@ -748,7 +748,7 @@ def apply_scim_filter(
 
         # Returns the mapped-to-your-domain column name(s) that a filter string
         # is operating upon, in an Array. If empty, the attribute is to be
-        # ignored. Raises an exception if entirey unmapped (thus unsupported).
+        # ignored. Raises an exception if entirely unmapped (thus unsupported).
         #
         # Note plural - the return value is always an array any of which should
         # be used (implicit 'OR').

app/models/scimitar/resource_type.rb

@@ -17,12 +17,10 @@ def initialize(attributes = {})
 
     def as_json(options = {})
       without_extensions = super(except: 'schemaExtensions')
-      if schemaExtensions.present?
-        extensions = schemaExtensions.map{|extension| {"schema" => extension, "required" => false}}
-        without_extensions.merge('schemaExtensions' => extensions)
-      else
-        without_extensions
-      end
+      return without_extensions unless schemaExtensions.present? # NOTE EARLY EXIT
+
+      extensions = schemaExtensions.map{|extension| {"schema" => extension, "required" => false}}
+      without_extensions.merge('schemaExtensions' => extensions)
     end
 
   end

app/models/scimitar/resources/base.rb

@@ -158,7 +158,7 @@ def constantize_complex_types(hash)
         hash.with_indifferent_access.each_pair do |attr_name, attr_value|
           scim_attribute = self.class.complex_scim_attributes[attr_name].try(:first)
 
-          if scim_attribute && scim_attribute.complexType
+          if scim_attribute&.complexType
             if scim_attribute.multiValued
               self.send("#{attr_name}=", attr_value&.map {|attr_for_each_item| complex_type_from_hash(scim_attribute, attr_for_each_item)})
             else

app/models/scimitar/resources/mixin.rb

@@ -473,7 +473,7 @@ def from_scim_patch!(patch_hash:)
             path_str = operation['path' ]
             value    = operation['value']
 
-            unless ['add', 'remove', 'replace'].include?(nature)
+            unless %w[add remove replace].include?(nature)
               raise Scimitar::InvalidSyntaxError.new("Unrecognised PATCH \"op\" value of \"#{nature}\"")
             end
 
@@ -613,7 +613,7 @@ def to_scim_backend(
                 built_dynamic_list = false
                 mapped_array = attrs_map_or_leaf_value.map do |value|
                   if ! value.is_a?(Hash)
-                    raise 'Bad attribute map: Array contains someting other than mapping Hash(es)'
+                    raise 'Bad attribute map: Array contains something other than mapping Hash(es)'
 
                   elsif value.key?(:match) # Static map
                     static_hash = { value[:match] => value[:with] }
@@ -741,7 +741,7 @@ def to_scim_backend(
           # +path+::                    Array of SCIM attribute names giving a
           #                             path into the SCIM schema where
           #                             iteration has reached. Used to find the
-          #                             schema attribute definiton and check
+          #                             schema attribute definition and check
           #                             mutability before writing.
           #
           def from_scim_backend!(
@@ -1088,7 +1088,7 @@ def from_patch_backend_apply!(nature:, path:, value:, altering_hash:, with_attr_
                     # associated collection or clearing a local model attribute
                     # directly to "nil").
                     #
-                    if handled == false
+                    unless handled
                       current_data_at_path[matched_index] = nil
                       compact_after = true
                     end
@@ -1290,7 +1290,7 @@ def from_patch_backend_apply!(nature:, path:, value:, altering_hash:, with_attr_
                       end
                     end
 
-                    if handled == false
+                    unless handled
                       altering_hash[path_component] = []
                     end
 
@@ -1445,7 +1445,7 @@ def all_matching_filter(filter:, within_array:, &block)
           #     { value: :work_email }
           #
           # If there was a SCIM entry with a type of something unrecognised,
-          # such as 'holday', then +nil+ would be returned since there is no
+          # such as 'holiday', then +nil+ would be returned since there is no
           # matching attribute map entry.
           #
           # Note that the <tt>:with_attr_map</tt> array can contain dynamic

app/models/scimitar/schema/base.rb

@@ -36,7 +36,7 @@ def self.cloned_scim_attributes
         scim_attributes.map { |scim_attribute| scim_attribute.clone }
       end
 
-      # Find a given attribute this schema, travelling down a path to any
+      # Find a given attribute of this schema, travelling down a path to any
       # sub-attributes within. Given that callers might be dealing with paths
       # into actual SCIM data, array indices for multi-value attributes are
       # allowed (as integers) and simply skipped - only the names are of

docker-compose.yml

@@ -1,9 +1,8 @@
-version: '2.3'
 services:
   postgresql:
     image: postgres:15
     ports:
-      - 5432
+      - '5432'
     environment:
       - POSTGRES_PASSWORD=Sup3rSecr1tPasw8rd
       - POSTGRES_USER=postgres

spec/models/scimitar/resources/mixin_spec.rb

@@ -555,7 +555,7 @@ def self.scim_timestamps_map
 
           expect do
             scim = instance.to_scim(location: 'https://test.com/static_map_test')
-          end.to raise_error(RuntimeError) { |e| expect(e.message).to include('Array contains someting other than mapping Hash(es)') }
+          end.to raise_error(RuntimeError) { |e| expect(e.message).to include('Array contains something other than mapping Hash(es)') }
         end
 
         it 'complains about bad Hash entries in mapping Arrays' do