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

Add import_directives #1158

Merged

Conversation

kdawgwilk
Copy link
Contributor

@kdawgwilk kdawgwilk commented Feb 16, 2022

There is some duplication between import_types but this PR is still relatively small so I wasnt sure if I should spend much time refactoring this to reduce some of the duplication

Closes #1152

@kdawgwilk kdawgwilk force-pushed the feature/import_directives branch 2 times, most recently from 9c7821a to d4c7a11 Compare February 16, 2022 20:44
@maartenvanvliet
Copy link
Contributor

What do you think about moving the directive imports to a separate DirectiveImports phase? So each phase does only one thing and makes it easier to remove/change the separate schema phases.

Same applies to type_extensions in #1157 btw. Should perhaps also be moved to a separate phase.

lib/absinthe/schema/notation.ex Outdated Show resolved Hide resolved
lib/absinthe/phase/schema/type_imports.ex Outdated Show resolved Hide resolved
@kdawgwilk
Copy link
Contributor Author

Alright got that feedback in and pulled that into a new DirectiveImports phase 👍

@benwilson512
Copy link
Contributor

@kdawgwilk this looks solid, let's rebase on the new master branch that has the formatter changes.

Copy link
Contributor

@benwilson512 benwilson512 left a comment

Choose a reason for hiding this comment

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

One small thing: Can you add a changelog entry for this so that it stays up to date?

@kdawgwilk
Copy link
Contributor Author

This is now rebased and changelog entry added 👍

@kdawgwilk
Copy link
Contributor Author

Anything else I should include here?

@benwilson512 benwilson512 merged commit e869dbb into absinthe-graphql:master Mar 11, 2022
@benwilson512
Copy link
Contributor

This is great, thank you!

@kdawgwilk kdawgwilk deleted the feature/import_directives branch March 12, 2022 00:38
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.

Add generic import_directives for macro based schemas
3 participants