Merge branch 'ld-graphql-description-cop-periods' into 'master'

Enforce `.` at end of GraphQL descriptions

See merge request gitlab-org/gitlab!46721

.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