Is it possible to make findBreakingChange return line numbers?

[JCMais]

This would help to create some tools, like github pull request checks.

[leebyron]

That would be awesome! I await your PR :)

[JCMais]

I will try to work on a POC for this and open a PR to receive feedback, my plan is to expose both ast nodes, previous one (if any), and new one (if any), depending on the ones that makes sense to the change.

The thing is, the ast nodes are only available when building the schema from a SDL. The tests currently works by creating the types directly, should I create another describe section for the same tests but using SDL with parse? Or create a separated test file (the current one is really big already)?

Other possibility is to change the current tests.

[JCMais]

Example of patch for the above:

Index: src/utilities/__tests__/findBreakingChanges-test.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/utilities/__tests__/findBreakingChanges-test.js (revision 4f92f55a74477fde3b00eb99f9a37f1ff2d1bd0d)
+++ src/utilities/__tests__/findBreakingChanges-test.js (date 1529630749674)
@@ -7,6 +7,7 @@

 import { expect } from 'chai';
 import { describe, it } from 'mocha';
+import { parse } from '../../language';
 import {
   GraphQLBoolean,
   GraphQLEnumType,
@@ -20,6 +21,7 @@
   GraphQLInt,
   GraphQLNonNull,
 } from '../../type';
+import { buildASTSchema } from '../buildASTSchema';
 import {
   BreakingChangeType,
   DangerousChangeType,
@@ -61,27 +63,29 @@
   });

   it('should detect if a type was removed or not', () => {
-    const type1 = new GraphQLObjectType({
-      name: 'Type1',
-      fields: {
-        field1: { type: GraphQLString },
-      },
-    });
-    const type2 = new GraphQLObjectType({
-      name: 'Type2',
-      fields: {
-        field1: { type: GraphQLString },
-      },
-    });
-
-    const oldSchema = new GraphQLSchema({
-      query: queryType,
-      types: [type1, type2],
-    });
-    const newSchema = new GraphQLSchema({
-      query: queryType,
-      types: [type2],
-    });
+    const oldSchema = buildASTSchema(
+      parse(`
+        type Query {
+          field1: String
+        }
+        type Type1 {
+          field1: String
+        }
+        type Type2 {
+          field1: String
+        }
+      `),
+    );
+    const newSchema = buildASTSchema(
+      parse(`
+        type Query {
+          field1: String
+        }
+        type Type1 {
+          field1: String
+        }
+      `),
+    );
     expect(findRemovedTypes(oldSchema, newSchema)).to.eql([
       {
         type: BreakingChangeType.TYPE_REMOVED,

[IvanGoncharov]

I will try to work on a POC for this and open a PR to receive feedback, my plan is to expose both ast nodes, previous one (if any), and new one (if any), depending on the ones that makes sense to the change.

@JCMais Sounds great 👍 I would recommend you to open WIP PR.

The thing is, the ast nodes are only available when building the schema from a SDL. The tests currently works by creating the types directly, should I create another describe section for the same tests but using SDL with parse? Or create a separated test file (the current one is really big already)?

I think such tests would benefit from using SDL and they were written using GraphQLSchema just because SDL was the experimental feature at that time. Also instead of buildASTSchema(parse(sdl)) you can call buildSchema that accepts SDL string directly. To make such changes easier to review can you please submit tests conversion as a separate PR. Please convert a couple of test cases and open a PR so you would get feedback early on.

[JCMais]

To make such changes easier to review can you please submit tests conversion as a separate PR.

will do that this week, thanks for the feedback.

[elevenpassin]

@leebyron This issue seems to be resolved, close it? :3

[JCMais]

@buoyantair it's not resolved, I've only updated the tests to build from SDL instead of using type constructors, the feature mentioned on this issue is still missing, didn't have time to finish it yet