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

Refactor instruction definitions #644

Draft
wants to merge 4 commits into
base: improve-factory
Choose a base branch
from

Conversation

gnawf
Copy link
Collaborator

@gnawf gnawf commented Dec 2, 2024

Just renaming stuff for clarity e.g.

  • NadelDefinitionRegistry -> NadelTypeDefinitionRegistry
  • NadelDefinition -> NadelInstructionDefinition
  • Created NadelInstructionDefinitionRegistry to hold the NadelInstructionDefinition and util functions.

Pending #638

Copy link

github-actions bot commented Dec 2, 2024

Test Results

  134 files  ±0  134 suites  ±0   58s ⏱️ +2s
  999 tests ±0  934 ✅ ±0  65 💤 ±0  0 ❌ ±0 
1 007 runs  ±0  942 ✅ ±0  65 💤 ±0  0 ❌ ±0 

Results for commit ba57d73. ± Comparison against base commit a19a800.

♻️ This comment has been updated with latest results.

context(NadelValidationContext)
private fun visitElement(schemaElement: NadelServiceSchemaElement): Boolean {
// Returns true if the element was added i.e. haven't visited before
return visitedTypes.add(schemaElement.toRef())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved visitElement into NadelValidationContext and made visitedTypes private.

@@ -12,7 +12,7 @@ import java.util.Collections
* Alternative to [graphql.schema.idl.TypeDefinitionRegistry] but is more generic
* and tailored to Nadel specific operations to build the overall schema.
*/
class NadelDefinitionRegistry {
class NadelTypeDefinitionRegistry {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed for clarity as I have also created a NadelInstructionDefinitionRegistry

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Holds the instruction definitions and utils.

This code was all moved from NadelValidationContext

Might be slightly refactored, but same otherwise.

@@ -17,7 +17,7 @@ class NadelAssignableTypeValidation internal constructor(
requiredType = overallType,
// Compare underlying type names
suppliedTypeName = underlyingType.unwrapAll().name,
requiredTypeName = getUnderlyingTypeName(overallType.unwrapAll()),
requiredTypeName = instructionDefinitions.getUnderlyingTypeName(overallType.unwrapAll()),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the util into NadelInstructionDefinitionRegistry

@@ -20,180 +11,16 @@ data class NadelValidationContext internal constructor(
val hydrationUnions: Set<String>,
val namespaceTypeNames: Set<String>,
val combinedTypeNames: Set<String>,
val definitions: Map<NadelSchemaMemberCoordinates, List<NadelDefinition>>,
val instructionDefinitions: NadelInstructionDefinitionRegistry,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created NadelInstructionDefinitionRegistry holder and move all the isHydrated etc methods into it

@@ -0,0 +1,3 @@
package graphql.nadel.definition

interface NadelInstructionDefinition
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed from NadelDefinition

File too small for Git to pick up on the rename.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant