Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused scalars from SDL #1423

Merged
merged 24 commits into from Feb 21, 2021
Merged

Conversation

sungam3r
Copy link
Member

According to the specification, it is not necessary to print unused types (in particular scalars).

Relates to graphql/graphql-spec#648

@sungam3r sungam3r changed the title Remove unused scalars from SDL Nov 17, 2019
@sungam3r sungam3r added the enhancement New feature or request label Dec 1, 2019
@sungam3r sungam3r changed the title Remove unused scalars from SDL [WIP] Remove unused scalars from SDL Dec 4, 2019
…o unused-scalars

# Conflicts:
#	src/GraphQL.ApiTests/ApiApprovalTests.PublicApi.GraphQL.approved.txt
#	src/GraphQL/Introspection/SchemaMetaFieldType.cs
@sungam3r sungam3r changed the title [WIP] Remove unused scalars from SDL Remove unused scalars from SDL Jan 18, 2020
@sungam3r
Copy link
Member Author

PR is ready for review.

Summary changes after information from graphql/graphql-spec#597 :

  1. Introspection response does not return ALL unused types (which essentially means not supported types): built-in scalars, custom scalars, graph types.
  2. SchemaPrinter does the same as (1.) + also does not print all built-in scalars at all.
  3. SchemaPrinter.IsDefinedType can be overridden to allow printing more types. Example - AllTypesSchemaPrinter from tests.

These changes are in accordance with the specification. @joemcbride, can you do a review, please?

@sungam3r sungam3r added the BREAKING Breaking changes in either public API or runtime behavior label Feb 24, 2020
@sungam3r
Copy link
Member Author

This PR is no longer needed after rework of GraphTypesLookup in #1887 . However, there will be one small change, maybe later in separate PR:

  1. SchemaPrinter does the same as (1.) + also does not print all built-in scalars at all.

…o unused-scalars

# Conflicts:
#	src/GraphQL.ApiTests/ApiApprovalTests.PublicApi.GraphQL.approved.txt
#	src/GraphQL.StarWars/IoC/SimpleContainer.cs
#	src/GraphQL.Tests/Introspection/IntrospectionResult.cs
#	src/GraphQL.Tests/StarWars/StarWarsIntrospectionTests.cs
#	src/GraphQL.Tests/Utilities/SchemaPrinterTests.cs
@sungam3r sungam3r mentioned this pull request Jan 27, 2021
…to unused-scalars

# Conflicts:
#	src/GraphQL.ApiTests/GraphQL.approved.txt
#	src/GraphQL.Tests/Utilities/SchemaPrinterTests.cs
#	src/GraphQL/GraphQLExtensions.cs
#	src/GraphQL/Introspection/SchemaMetaFieldType.cs
#	src/GraphQL/Utilities/SchemaPrinter.cs
@sungam3r sungam3r changed the base branch from master to develop February 20, 2021 21:57
@sungam3r
Copy link
Member Author

OK. As I expected, all scalars are already printed correctly thanks to our edits in other PRs.

@sungam3r
Copy link
Member Author

@Shane32 PR is ready for review.

@codecov-io
Copy link

codecov-io commented Feb 21, 2021

Codecov Report

Merging #1423 (3fa0bf0) into develop (c277b10) will decrease coverage by 0.01%.
The diff coverage is 96.55%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1423      +/-   ##
===========================================
- Coverage    84.07%   84.06%   -0.02%     
===========================================
  Files          371      371              
  Lines        13098    13076      -22     
  Branches      1937     1935       -2     
===========================================
- Hits         11012    10992      -20     
+ Misses        1555     1553       -2     
  Partials       531      531              
Impacted Files Coverage Δ
src/GraphQL/GraphQLExtensions.cs 88.88% <94.44%> (+0.67%) ⬆️
...hQL/Utilities/Federation/FederatedSchemaPrinter.cs 85.13% <100.00%> (ø)
src/GraphQL/Utilities/SchemaPrinter.cs 84.01% <100.00%> (-0.74%) ⬇️
src/GraphQL/Utilities/SchemaPrinterOptions.cs 100.00% <100.00%> (ø)
src/GraphQL/Types/Schema.cs 88.39% <0.00%> (-1.11%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c277b10...4610bf9. Read the comment docs.

Copy link
Member

@Shane32 Shane32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only have one minor comment. Please read and then merge at your discretion @sungam3r

src/GraphQL/GraphQLExtensions.cs Outdated Show resolved Hide resolved
src/GraphQL/Utilities/Federation/FederatedSchemaPrinter.cs Outdated Show resolved Hide resolved
@sungam3r sungam3r merged commit ad980bd into graphql-dotnet:develop Feb 21, 2021
@sungam3r sungam3r deleted the unused-scalars branch February 21, 2021 21:31
@sungam3r sungam3r added this to the 4.0 milestone Feb 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING Breaking changes in either public API or runtime behavior enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants