GraphQL enforce `.` at end of description strings

Previously our GraphQL styleguide stated that description strings should
not end in periods. Descriptions provided by `ruby-graphql` objects all
do end in periods, so our overall API is inconsistent in how it presents
its schema descriptions.

This change switches our description styleguide to recommend that we add
periods to the end of description strings.

It also adds to the existing `GraphQL/Descriptions` cop to enforce this
which can auto-correct offenses.

.rubocop.yml

@@ -427,6 +427,7 @@ Scalability/FileUploads:
 
 Graphql/Descriptions:
   Enabled: true
+  AutoCorrect: true
   Include:
     - 'app/graphql/**/*'
     - 'ee/app/graphql/**/*'

app/graphql/types/base_field.rb

@@ -78,7 +78,7 @@ module Types
     attr_reader :feature_flag
 
     def feature_documentation_message(key, description)
-      "#{description}. Available only when feature flag `#{key}` is enabled"
+      "#{description} Available only when feature flag `#{key}` is enabled."
     end
 
     def check_feature_flag(args)

app/graphql/types/concerns/gitlab_style_deprecations.rb

@@ -23,8 +23,8 @@ module GitlabStyleDeprecations
     raise ArgumentError, '`milestone` must be a `String`' unless milestone.is_a?(String)
 
     deprecated_in = "Deprecated in #{milestone}"
-    kwargs[:deprecation_reason] = "#{reason}. #{deprecated_in}"
-    kwargs[:description] += ". #{deprecated_in}: #{reason}" if kwargs[:description]
+    kwargs[:deprecation_reason] = "#{reason}. #{deprecated_in}."
+    kwargs[:description] += " #{deprecated_in}: #{reason}." if kwargs[:description]
 
     kwargs
   end

doc/development/api_graphql_styleguide.md

@@ -310,7 +310,7 @@ class MergeRequestPermissionsType < BasePermissionType
   abilities :admin_merge_request, :update_merge_request, :create_note
 
   ability_field :resolve_note,
-                description: 'Indicates the user can resolve discussions on the merge request'
+                description: 'Indicates the user can resolve discussions on the merge request.'
   permission_field :push_to_source_branch, method: :can_push_to_source_branch?
 end
 ```
@@ -369,7 +369,7 @@ Example:
 ```ruby
 field :test_field, type: GraphQL::STRING_TYPE,
       null: true,
-      description: 'Some test field',
+      description: 'Some test field.',
       feature_flag: :my_feature_flag
 ```
 
@@ -394,7 +394,7 @@ Example:
 field :foo, GraphQL::STRING_TYPE,
       null: true,
       description: 'Some test field. Will always return `null`' \
-                   'if `my_feature_flag` feature flag is disabled'
+                   'if `my_feature_flag` feature flag is disabled.'
 
 def foo
   object.foo if Feature.enabled?(:my_feature_flag, object)
@@ -420,7 +420,7 @@ Example:
 ```ruby
 field :token, GraphQL::STRING_TYPE, null: true,
       deprecated: { reason: 'Login via token has been removed', milestone: '10.0' },
-      description: 'Token for login'
+      description: 'Token for login.'
 ```
 
 The original `description` of the things being deprecated should be maintained,
@@ -441,7 +441,7 @@ Example:
 ```ruby
 field :designs, ::Types::DesignManagement::DesignCollectionType, null: true,
       deprecated: { reason: 'Use `designCollection`', milestone: '10.0' },
-      description: 'The designs associated with this issue',
+      description: 'The designs associated with this issue.',
 ```
 
 ```ruby
@@ -477,9 +477,9 @@ module Types
     graphql_name 'TrafficLightState'
     description 'State of a traffic light'
 
-    value 'RED', description: 'Drivers must stop'
-    value 'YELLOW', description: 'Drivers must stop when it is safe to'
-    value 'GREEN', description: 'Drivers can start or keep driving'
+    value 'RED', description: 'Drivers must stop.'
+    value 'YELLOW', description: 'Drivers must stop when it is safe to.'
+    value 'GREEN', description: 'Drivers can start or keep driving.'
   end
 end
 ```
@@ -498,8 +498,8 @@ module Types
     graphql_name 'EpicState'
     description 'State of a GitLab epic'
 
-    value 'OPENED', value: 'opened', description: 'An open Epic'
-    value 'CLOSED', value: 'closed', description: 'An closed Epic'
+    value 'OPENED', value: 'opened', description: 'An open Epic.'
+    value 'CLOSED', value: 'closed', description: 'A closed Epic.'
   end
 end
 ```
@@ -523,7 +523,7 @@ module Types
     description 'Incident severity'
 
     ::IssuableSeverity.severities.keys.each do |severity|
-      value severity.upcase, value: severity, description: "#{severity.titleize} severity"
+      value severity.upcase, value: severity, description: "#{severity.titleize} severity."
     end
   end
 end
@@ -562,15 +562,15 @@ We can use GraphQL types like this:
 ```ruby
 module Types
   class ChartType < BaseObject
-    field :title, GraphQL::STRING_TYPE, null: true, description: 'Title of the chart'
-    field :data, [Types::ChartDatumType], null: true, description: 'Data of the chart'
+    field :title, GraphQL::STRING_TYPE, null: true, description: 'Title of the chart.'
+    field :data, [Types::ChartDatumType], null: true, description: 'Data of the chart.'
   end
 end
 
 module Types
   class ChartDatumType < BaseObject
-    field :x, GraphQL::INT_TYPE, null: true, description: 'X-axis value of the chart datum'
-    field :y, GraphQL::INT_TYPE, null: true, description: 'Y-axis value of the chart datum'
+    field :x, GraphQL::INT_TYPE, null: true, description: 'X-axis value of the chart datum.'
+    field :y, GraphQL::INT_TYPE, null: true, description: 'Y-axis value of the chart datum.'
   end
 end
 ```
@@ -584,7 +584,7 @@ A description of a field or argument is given using the `description:`
 keyword. For example:
 
 ```ruby
-field :id, GraphQL::ID_TYPE, description: 'ID of the resource'
+field :id, GraphQL::ID_TYPE, description: 'ID of the resource.'
 ```
 
 Descriptions of fields and arguments are viewable to users through:
@@ -606,14 +606,14 @@ descriptions:
 - Always include the word `"timestamp"` when describing an argument or
   field of type `Types::TimeType`. This lets the reader know that the
   format of the property is `Time`, rather than just `Date`.
-- No `.` at end of strings.
+- Must end with a period (`.`).
 
 Example:
 
 ```ruby
-field :id, GraphQL::ID_TYPE, description: 'ID of the issue'
-field :confidential, GraphQL::BOOLEAN_TYPE, description: 'Indicates the issue is confidential'
-field :closed_at, Types::TimeType, description: 'Timestamp of when the issue was closed'
+field :id, GraphQL::ID_TYPE, description: 'ID of the issue.'
+field :confidential, GraphQL::BOOLEAN_TYPE, description: 'Indicates the issue is confidential.'
+field :closed_at, Types::TimeType, description: 'Timestamp of when the issue was closed.'
 ```
 
 ### `copy_field_description` helper
@@ -889,8 +889,8 @@ Then we can use these resolver on fields:
 ```ruby
 # In PipelineType
 
-field :jobs, resolver: JobsResolver, description: 'All jobs'
-field :job, resolver: JobsResolver.single, description: 'A single job'
+field :jobs, resolver: JobsResolver, description: 'All jobs.'
+field :job, resolver: JobsResolver.single, description: 'A single job.'
 ```
 
 ### Correct use of `Resolver#ready?`
@@ -965,7 +965,7 @@ to advertise the need for lookahead:
   field :my_things, MyThingType.connection_type, null: true,
         extras: [:lookahead], # Necessary
         resolver: MyThingResolver,
-        description: 'My things'
+        description: 'My things.'
 ```
 
 For an example of real world use, please
@@ -1034,7 +1034,7 @@ To find the parent object in your `Presenter` class:
      field :computed_field, SomeType, null: true,
            method: :my_computing_method,
            extras: [:parent], # Necessary
-           description: 'My field description'
+           description: 'My field description.'
 
      field :resolver_field, resolver: SomeTypeResolver
 
@@ -1042,7 +1042,7 @@ To find the parent object in your `Presenter` class:
 
      extras [:parent]
      type SomeType, null: true
-     description 'My field description'
+     description 'My field description.'
    ```
 
 1. Declare your field's method in your Presenter class and have it accept the `parent` keyword argument.
@@ -1161,7 +1161,7 @@ Example:
 ```ruby
 argument :my_arg, GraphQL::STRING_TYPE,
          required: true,
-         description: "A description of the argument"
+         description: "A description of the argument."
 ```
 
 Each GraphQL `argument` defined is passed to the `#resolve` method
@@ -1186,11 +1186,11 @@ defines these arguments (some
 ```ruby
 argument :project_path, GraphQL::ID_TYPE,
          required: true,
-         description: "The project the merge request to mutate is in"
+         description: "The project the merge request to mutate is in."
 
 argument :iid, GraphQL::STRING_TYPE,
          required: true,
-         description: "The IID of the merge request to mutate"
+         description: "The IID of the merge request to mutate."
 
 argument :wip,
          GraphQL::BOOLEAN_TYPE,
@@ -1242,7 +1242,7 @@ field:
 field :merge_request,
       Types::MergeRequestType,
       null: true,
-      description: "The merge request after mutation"
+      description: "The merge request after mutation."
 ```
 
 This means that the hash returned from `resolve` in this mutation
@@ -1527,7 +1527,7 @@ and handles time inputs.
 Example:
 
 ```ruby
-field :created_at, Types::TimeType, null: true, description: 'Timestamp of when the issue was created'
+field :created_at, Types::TimeType, null: true, description: 'Timestamp of when the issue was created.'
 ```
 
 ## Testing

ee/spec/graphql/types/mutation_type_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe GitlabSchema.types['Mutation'] do
 
     with_them do
       let(:field) { get_field(field_name) }
-      let(:deprecation_reason) { "#{reason}. Deprecated in #{milestone}" }
+      let(:deprecation_reason) { "#{reason}. Deprecated in #{milestone}." }
 
       it { expect(field).to be_present }
       it { expect(field.deprecation_reason).to eq(deprecation_reason) }

rubocop/cop/graphql/descriptions.rb

@@ -13,42 +13,74 @@
 #     argument :some_argument, GraphQL::STRING_TYPE
 #   end
 #
+#   class UngoodClass
+#     field :some_argument,
+#       GraphQL::STRING_TYPE,
+#       description: "A description that does not end in a period"
+#   end
+#
 #   # good
 #   class GreatClass
 #     argument :some_field,
 #       GraphQL::STRING_TYPE,
-#       description: "Well described - a superb description"
+#       description: "Well described - a superb description."
 #
 #     field :some_field,
 #       GraphQL::STRING_TYPE,
-#       description: "A thorough and compelling description"
+#       description: "A thorough and compelling description."
 #   end
 
 module RuboCop
   module Cop
     module Graphql
       class Descriptions < RuboCop::Cop::Cop
-        MSG = 'Please add a `description` property.'
+        MSG_NO_DESCRIPTION = 'Please add a `description` property.'
+        MSG_NO_PERIOD = '`description` strings must end with a `.`.'
 
         # ability_field and permission_field set a default description.
-        def_node_matcher :fields, <<~PATTERN
-          (send nil? :field $...)
-        PATTERN
-
-        def_node_matcher :arguments, <<~PATTERN
-          (send nil? :argument $...)
+        def_node_matcher :field_or_argument?, <<~PATTERN
+          (send nil? {:field :argument} ...)
         PATTERN
 
-        def_node_matcher :has_description?, <<~PATTERN
-          (hash <(pair (sym :description) _) ...>)
+        def_node_matcher :description, <<~PATTERN
+          (... (hash <(pair (sym :description) $_) ...>))
         PATTERN
 
         def on_send(node)
-          matches = fields(node) || arguments(node)
+          return unless field_or_argument?(node)
+
+          description = description(node)
+
+          return add_offense(node, location: :expression, message: MSG_NO_DESCRIPTION) unless description
+
+          add_offense(node, location: :expression, message: MSG_NO_PERIOD) if no_period?(description)
+        end
+
+        # Autocorrect missing periods at end of description.
+        def autocorrect(node)
+          lambda do |corrector|
+            description = description(node)
+            next unless description
+
+            corrector.insert_after(before_end_quote(description), '.')
+          end
+        end
+
+        private
+
+        def no_period?(description)
+          # Test that the description node is a `:str` (as opposed to
+          # a `#copy_field_description` call) before checking.
+          description.type == :str && !description.value.strip.end_with?('.')
+        end
 
-          return if matches.nil?
+        # Returns a Parser::Source::Range that ends just before the final String delimiter.
+        def before_end_quote(string)
+          return string.source_range.adjust(end_pos: -1) unless string.heredoc?
 
-          add_offense(node, location: :expression) unless has_description?(matches.last)
+          heredoc_source = string.location.heredoc_body.source
+          adjust = heredoc_source.index(/\s+\Z/) - heredoc_source.length
+          string.location.heredoc_body.adjust(end_pos: adjust)
         end
       end
     end

spec/graphql/types/base_field_spec.rb

@@ -145,11 +145,11 @@ RSpec.describe Types::BaseField do
 
   describe '#description' do
     context 'feature flag given' do
-      let(:field) { described_class.new(name: 'test', type: GraphQL::STRING_TYPE, feature_flag: flag, null: false, description: 'Test description') }
+      let(:field) { described_class.new(name: 'test', type: GraphQL::STRING_TYPE, feature_flag: flag, null: false, description: 'Test description.') }
       let(:flag) { :test_flag }
 
       it 'prepends the description' do
-        expect(field.description). to eq 'Test description. Available only when feature flag `test_flag` is enabled'
+        expect(field.description). to eq 'Test description. Available only when feature flag `test_flag` is enabled.'
       end
 
       context 'falsey feature_flag values' do
@@ -164,7 +164,7 @@ RSpec.describe Types::BaseField do
 
         with_them do
           it 'returns the correct description' do
-            expect(field.description).to eq('Test description')
+            expect(field.description).to eq('Test description.')
           end
         end
       end
@@ -181,11 +181,11 @@ RSpec.describe Types::BaseField do
     it 'interacts well with the `feature_flag` property' do
       field = subject(
         deprecated: { milestone: '1.10', reason: 'Deprecation reason' },
-        description: 'Field description',
+        description: 'Field description.',
         feature_flag: 'foo_flag'
       )
 
-      expectation = 'Field description. Available only when feature flag `foo_flag` is enabled. Deprecated in 1.10: Deprecation reason'
+      expectation = 'Field description. Available only when feature flag `foo_flag` is enabled. Deprecated in 1.10: Deprecation reason.'
 
       expect(field.description).to eq(expectation)
     end

spec/lib/gitlab/graphql/docs/renderer_spec.rb

@@ -30,7 +30,7 @@ RSpec.describe Gitlab::Graphql::Docs::Renderer do
         Class.new(Types::BaseObject) do
           graphql_name 'ArrayTest'
 
-          field :foo, [GraphQL::STRING_TYPE], null: false, description: 'A description'
+          field :foo, [GraphQL::STRING_TYPE], null: false, description: 'A description.'
         end
       end
 
@@ -40,7 +40,7 @@ RSpec.describe Gitlab::Graphql::Docs::Renderer do
 
           | Field | Type | Description |
           | ----- | ---- | ----------- |
-          | `foo` | String! => Array | A description |
+          | `foo` | String! => Array | A description. |
         DOC
 
         is_expected.to include(expectation)
@@ -52,8 +52,8 @@ RSpec.describe Gitlab::Graphql::Docs::Renderer do
         Class.new(Types::BaseObject) do
           graphql_name 'OrderingTest'
 
-          field :foo, GraphQL::STRING_TYPE, null: false, description: 'A description of foo field'
-          field :bar, GraphQL::STRING_TYPE, null: false, description: 'A description of bar field'
+          field :foo, GraphQL::STRING_TYPE, null: false, description: 'A description of foo field.'
+          field :bar, GraphQL::STRING_TYPE, null: false, description: 'A description of bar field.'
         end
       end
 
@@ -63,8 +63,8 @@ RSpec.describe Gitlab::Graphql::Docs::Renderer do
 
           | Field | Type | Description |
           | ----- | ---- | ----------- |
-          | `bar` | String! | A description of bar field |
-          | `foo` | String! | A description of foo field |
+          | `bar` | String! | A description of bar field. |
+          | `foo` | String! | A description of foo field. |
         DOC
 
         is_expected.to include(expectation)
@@ -76,7 +76,7 @@ RSpec.describe Gitlab::Graphql::Docs::Renderer do
         Class.new(Types::BaseObject) do
           graphql_name 'DeprecatedTest'
 
-          field :foo, GraphQL::STRING_TYPE, null: false, deprecated: { reason: 'This is deprecated', milestone: '1.10' }, description: 'A description'
+          field :foo, GraphQL::STRING_TYPE, null: false, deprecated: { reason: 'This is deprecated', milestone: '1.10' }, description: 'A description.'
         end
       end
 
@@ -86,7 +86,7 @@ RSpec.describe Gitlab::Graphql::Docs::Renderer do
 
           | Field | Type | Description |
           | ----- | ---- | ----------- |
-          | `foo` **{warning-solid}** | String! | **Deprecated:** This is deprecated. Deprecated in 1.10 |
+          | `foo` **{warning-solid}** | String! | **Deprecated:** This is deprecated. Deprecated in 1.10. |
         DOC
 
         is_expected.to include(expectation)
@@ -98,14 +98,14 @@ RSpec.describe Gitlab::Graphql::Docs::Renderer do
         enum_type = Class.new(Types::BaseEnum) do
           graphql_name 'MyEnum'
 
-          value 'BAZ', description: 'A description of BAZ'
-          value 'BAR', description: 'A description of BAR', deprecated: { reason: 'This is deprecated', milestone: '1.10' }
+          value 'BAZ', description: 'A description of BAZ.'
+          value 'BAR', description: 'A description of BAR.', deprecated: { reason: 'This is deprecated', milestone: '1.10' }
         end
 
         Class.new(Types::BaseObject) do
           graphql_name 'EnumTest'
 
-          field :foo, enum_type, null: false, description: 'A description of foo field'
+          field :foo, enum_type, null: false, description: 'A description of foo field.'
         end
       end
 
@@ -115,8 +115,8 @@ RSpec.describe Gitlab::Graphql::Docs::Renderer do
 
           | Value | Description |
           | ----- | ----------- |
-          | `BAR` **{warning-solid}** | **Deprecated:** This is deprecated. Deprecated in 1.10 |
-          | `BAZ` | A description of BAZ |
+          | `BAR` **{warning-solid}** | **Deprecated:** This is deprecated. Deprecated in 1.10. |
+          | `BAZ` | A description of BAZ. |
         DOC
 
         is_expected.to include(expectation)

spec/rubocop/cop/graphql/descriptions_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe RuboCop::Cop::Graphql::Descriptions, type: :rubocop do
   subject(:cop) { described_class.new }
 
   context 'fields' do
-    it 'adds an offense when there is no field description' do
+    it 'adds an offense when there is no description' do
       inspect_source(<<~TYPE)
         module Types
           class FakeType < BaseObject
@@ -24,24 +24,37 @@ RSpec.describe RuboCop::Cop::Graphql::Descriptions, type: :rubocop do
       expect(cop.offenses.size).to eq 1
     end
 
-    it 'does not add an offense for fields with a description' do
-      expect_no_offenses(<<~TYPE.strip)
+    it 'adds an offense when description does not end in a period' do
+      inspect_source(<<~TYPE)
         module Types
           class FakeType < BaseObject
-            graphql_name 'FakeTypeName'
-
-            argument :a_thing,
+            field :a_thing,
               GraphQL::STRING_TYPE,
               null: false,
               description: 'A descriptive description'
           end
         end
       TYPE
+
+      expect(cop.offenses.size).to eq 1
+    end
+
+    it 'does not add an offense when description is correct' do
+      expect_no_offenses(<<~TYPE.strip)
+        module Types
+          class FakeType < BaseObject
+            field :a_thing,
+              GraphQL::STRING_TYPE,
+              null: false,
+              description: 'A descriptive description.'
+          end
+        end
+      TYPE
     end
   end
 
   context 'arguments' do
-    it 'adds an offense when there is no argument description' do
+    it 'adds an offense when there is no description' do
       inspect_source(<<~TYPE)
         module Types
           class FakeType < BaseObject
@@ -55,19 +68,88 @@ RSpec.describe RuboCop::Cop::Graphql::Descriptions, type: :rubocop do
       expect(cop.offenses.size).to eq 1
     end
 
-    it 'does not add an offense for arguments with a description' do
-      expect_no_offenses(<<~TYPE.strip)
+    it 'adds an offense when description does not end in a period' do
+      inspect_source(<<~TYPE)
         module Types
           class FakeType < BaseObject
-            graphql_name 'FakeTypeName'
+            argument :a_thing,
+              GraphQL::STRING_TYPE,
+              null: false,
+              description: 'Behold! A description'
+          end
+        end
+      TYPE
 
+      expect(cop.offenses.size).to eq 1
+    end
+
+    it 'does not add an offense when description is correct' do
+      expect_no_offenses(<<~TYPE.strip)
+        module Types
+          class FakeType < BaseObject
             argument :a_thing,
+              GraphQL::STRING_TYPE,
+              null: false,
+              description: 'Behold! A description.'
+          end
+        end
+      TYPE
+    end
+  end
+
+  describe 'autocorrecting descriptions without periods' do
+    it 'can autocorrect' do
+      expect_offense(<<~TYPE)
+        module Types
+          class FakeType < BaseObject
+            field :a_thing,
+            ^^^^^^^^^^^^^^^ `description` strings must end with a `.`.
               GraphQL::STRING_TYPE,
               null: false,
               description: 'Behold! A description'
           end
         end
       TYPE
+
+      expect_correction(<<~TYPE)
+        module Types
+          class FakeType < BaseObject
+            field :a_thing,
+              GraphQL::STRING_TYPE,
+              null: false,
+              description: 'Behold! A description.'
+          end
+        end
+      TYPE
+    end
+
+    it 'can autocorrect a heredoc' do
+      expect_offense(<<~TYPE)
+        module Types
+          class FakeType < BaseObject
+            field :a_thing,
+            ^^^^^^^^^^^^^^^ `description` strings must end with a `.`.
+              GraphQL::STRING_TYPE,
+              null: false,
+              description: <<~DESC
+                Behold! A description
+              DESC
+          end
+        end
+      TYPE
+
+      expect_correction(<<~TYPE)
+        module Types
+          class FakeType < BaseObject
+            field :a_thing,
+              GraphQL::STRING_TYPE,
+              null: false,
+              description: <<~DESC
+                Behold! A description.
+              DESC
+          end
+        end
+      TYPE
     end
   end
 end

spec/support/shared_examples/graphql/types/gitlab_style_deprecations_shared_examples.rb

@@ -32,16 +32,16 @@ RSpec.shared_examples 'Gitlab-style deprecations' do
   it 'adds a formatted `deprecated_reason` to the subject' do
     deprecable = subject(deprecated: { milestone: '1.10', reason: 'Deprecation reason' })
 
-    expect(deprecable.deprecation_reason).to eq('Deprecation reason. Deprecated in 1.10')
+    expect(deprecable.deprecation_reason).to eq('Deprecation reason. Deprecated in 1.10.')
   end
 
   it 'appends to the description if given' do
     deprecable = subject(
       deprecated: { milestone: '1.10', reason: 'Deprecation reason' },
-      description: 'Deprecable description'
+      description: 'Deprecable description.'
     )
 
-    expect(deprecable.description).to eq('Deprecable description. Deprecated in 1.10: Deprecation reason')
+    expect(deprecable.description).to eq('Deprecable description. Deprecated in 1.10: Deprecation reason.')
   end
 
   it 'does not append to the description if it is absent' do