Respond to reviewer feedback

* Add documentation for public methods
* Remove pointless duplicated method call
* Be more specific about ignored file
* Improvements to equality tests

Requested by reviewer

.gitignore

@@ -86,4 +86,4 @@ jsdoc/
 .projections.json
 /qa/.rakeTasks
 webpack-dev-server.json
-.nvimrc
+/.nvimrc

app/graphql/gitlab_schema.rb

@@ -57,12 +57,23 @@ class GitlabSchema < GraphQL::Schema
       object.to_global_id
     end
 
+    # Find an object by looking it up from its global ID, passed as a string.
+    #
+    # This is the composition of 'parse_gid' and 'find_by_gid', see these
+    # methods for further documentation.
     def object_from_id(global_id, ctx = {})
       gid = parse_gid(global_id, ctx)
 
       find_by_gid(gid)
     end
 
+    # Find an object by looking it up from its 'GlobalID'.
+    #
+    # * For `ApplicationRecord`s, this is equivalent to
+    #   `global_id.model_class.find(gid.model_id)`, but more efficient.
+    # * For classes that implement `.lazy_find(global_id)`, this class method
+    #   will be called.
+    # * All other classes will use `GlobalID#find`
     def find_by_gid(gid)
       if gid.model_class < ApplicationRecord
         Gitlab::Graphql::Loaders::BatchModelLoader.new(gid.model_class, gid.model_id).find
@@ -73,6 +84,23 @@ class GitlabSchema < GraphQL::Schema
       end
     end
 
+    # Parse a string to a GlobalID, raising ArgumentError if there are problems
+    # with it.
+    #
+    # Problems that may occur:
+    #  * it may not be syntactically valid
+    #  * it may not match the expected type (see below)
+    #
+    # Options:
+    #  * :expected_type [Class] - the type of object this GlobalID should refer to.
+    #
+    # e.g.
+    #
+    # ```
+    #   gid = GitlabSchema.parse_gid(my_string, expected_type: ::Project)
+    #   project_id = gid.model_id
+    #   gid.model_class == ::Project
+    # ```
     def parse_gid(global_id, ctx = {})
       expected_type = ctx[:expected_type]
       gid = GlobalID.parse(global_id)

ee/app/models/design_management/design_at_version.rb

@@ -35,7 +35,7 @@ module DesignManagement
     end
 
     def ==(other)
-      return false unless other.is_a?(self.class)
+      return false unless other && self.class == other.class
 
       other.id == id
     end

ee/spec/models/design_management/design_at_version_spec.rb

@@ -19,32 +19,56 @@ describe DesignManagement::DesignAtVersion do
   end
 
   describe '#==' do
-    it 'has structural semantics' do
-      design = create(:design, issue: issue)
-      version = create(:design_version, designs: [design], issue: issue)
+    it 'identifies objects created with the same parameters as equal' do
+      design = build_stubbed(:design, issue: issue)
+      version = build_stubbed(:design_version, designs: [design], issue: issue)
 
-      this = create(:design_at_version, design: design, version: version)
-      other = create(:design_at_version, design: design, version: version)
+      this = build_stubbed(:design_at_version, design: design, version: version)
+      other = build_stubbed(:design_at_version, design: design, version: version)
 
       expect(this).to eq(other)
+      expect(other).to eq(this)
     end
 
-    it 'does not admit false positives' do
-      design = create(:design, issue: issue)
-      version_a = create(:design_version, designs: [design])
-      version_b = create(:design_version, designs: [design])
+    it 'identifies unequal objects as unequal, by virtue of their version' do
+      design = build_stubbed(:design, issue: issue)
+      version_a = build_stubbed(:design_version, designs: [design])
+      version_b = build_stubbed(:design_version, designs: [design])
 
-      this = create(:design_at_version, design: design, version: version_a)
-      other = create(:design_at_version, design: design, version: version_b)
+      this = build_stubbed(:design_at_version, design: design, version: version_a)
+      other = build_stubbed(:design_at_version, design: design, version: version_b)
 
+      expect(this).not_to eq(nil)
+      expect(this).not_to eq(design)
       expect(this).not_to eq(other)
+      expect(other).not_to eq(this)
+    end
+
+    it 'identifies unequal objects as unequal, by virtue of their design' do
+      design_a = build_stubbed(:design, issue: issue)
+      design_b = build_stubbed(:design, issue: issue)
+      version = build_stubbed(:design_version, designs: [design_a, design_b])
+
+      this = build_stubbed(:design_at_version, design: design_a, version: version)
+      other = build_stubbed(:design_at_version, design: design_b, version: version)
+
+      expect(this).not_to eq(other)
+      expect(other).not_to eq(this)
     end
 
     it 'rejects objects with the same id and the wrong class' do
-      dav = create(:design_at_version)
+      dav = build_stubbed(:design_at_version)
 
       expect(dav).not_to eq(OpenStruct.new(id: dav.id))
     end
+
+    it 'expects objects to be of the same type, not subtypes' do
+      subtype = Class.new(described_class)
+      dav = build_stubbed(:design_at_version)
+      other = subtype.new(design: dav.design, version: dav.version)
+
+      expect(dav).not_to eq(other)
+    end
   end
 
   describe 'status methods' do

ee/spec/support/shared_examples/design_fields.rb

 # frozen_string_literal: true
 
+# To use these shared examples, you may define a value in scope named
+# `extra_design_fields`, to pass any extra fields in addition to the
+# standard design fields.
 shared_examples 'a GraphQL type with design fields' do
+  let(:extra_design_fields) { [] }
+
   it { expect(described_class).to require_graphql_authorizations(:read_design) }
 
   it 'exposes the expected design fields' do
@@ -16,6 +21,6 @@ shared_examples 'a GraphQL type with design fields' do
       notes_count
     ] + extra_design_fields
 
-    is_expected.to have_graphql_fields(*expected_fields)
+    is_expected.to have_graphql_fields(*expected_fields).only
   end
 end

spec/support/helpers/graphql_helpers.rb

@@ -106,7 +106,7 @@ module GraphqlHelpers
 
   def query_graphql_field(name, attributes = {}, fields = nil)
     field_params = if attributes.present?
-                     "(#{attributes_to_graphql(attributes)})" if attributes.present?
+                     "(#{attributes_to_graphql(attributes)})"
                    else
                      ''
                    end