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

Rename @extraTag directive to @goTag and make repeatable #1680

Merged
merged 3 commits into from
Oct 30, 2021
Merged

Rename @extraTag directive to @goTag and make repeatable #1680

merged 3 commits into from
Oct 30, 2021

Conversation

wilhelmeek
Copy link
Contributor

@wilhelmeek wilhelmeek commented Oct 29, 2021

  • Rename for consistency with @goField and @goModel
  • Allow for repeatable use to bind multiple tag pairs

edit: added a comment down below, but contemplating whether it makes sense to have this directive at all given the addition of field-level hooks cc @johnmaguire

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@wilhelmeek wilhelmeek mentioned this pull request Oct 29, 2021
2 tasks
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 54.015% when pulling a0e45dc on wilhelmeek:goTags into 87f9e43 on 99designs:master.

@wilhelmeek
Copy link
Contributor Author

Hm, totally missed this great addition from @tprebs (thanks 🙏)

I actually like the hook based approach a lot more given the extra flexibility it affords. Partially tempted to revert @extraTag entirely and more heavily promote the hooks (maybe even include one in the repo). Definitely keen to hear thoughts and opinions

@wilhelmeek wilhelmeek marked this pull request as ready for review October 29, 2021 05:39
@tprebs
Copy link
Contributor

tprebs commented Oct 29, 2021

For our use case, I do like the hook based approach more as it allows your directive to be defined more as a behaviour rather than implementation detail. In our case we have a constraint directive (similar to https://www.apollographql.com/blog/backend/validation/graphql-validation-using-directives/) which defines the validation behaviour of the schema. This is abstract enough that we can switch how we do validation by swapping out the hook while the schema remains the same.

That being said, not everyone will have the same concerns as we do about abstraction and don't mind exposing implementation detail in the schema. Maybe a middle ground is re-implementing @goTag to be applied via a hook and include it as an example hook implementation in the repo. This should help promote the use of hooks while also providing a simple option for users to apply tags to the generated model.

Copy link
Collaborator

@StevenACoffman StevenACoffman left a comment

Choose a reason for hiding this comment

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

goTag is much better than "extraTag", but I do think re-implementing it to be applied via a hook would be a good example. Maybe we merge this PR, and make another one to change the implementation to use hooks?

@tprebs
Copy link
Contributor

tprebs commented Oct 29, 2021

I'm happy rewrite it it using hooks @StevenACoffman unless you want to pick it up @wilhelmeek?

@wilhelmeek
Copy link
Contributor Author

I'm happy rewrite it it using hooks @StevenACoffman unless you want to pick it up @wilhelmeek?

No, go ahead 😄 I'm a big fan of this middle ground

@StevenACoffman StevenACoffman merged commit 37a4e7e into 99designs:master Oct 30, 2021
@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Oct 30, 2021

Ok, I merged the rename, so @tprebs if you re-write it with hooks, that would be great to get in before a release. Thanks very much!

@wilhelmeek wilhelmeek deleted the goTags branch October 31, 2021 06:45
@tprebs
Copy link
Contributor

tprebs commented Oct 31, 2021

Ok, I merged the rename, so @tprebs if you re-write it with hooks, that would be great to get in before a release. Thanks very much!

Will create the PR first thing tomorrow morning

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.

4 participants