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

Reimplement goTag using FieldMutateHook #1682

Merged
merged 2 commits into from
Nov 1, 2021
Merged

Reimplement goTag using FieldMutateHook #1682

merged 2 commits into from
Nov 1, 2021

Conversation

tprebs
Copy link
Contributor

@tprebs tprebs commented Nov 1, 2021

This PR reimplements the extraTags/goTag directive as a fieldMutateHook and applies it as the default hook for the modelgen Plugin. This has been done as a request off the back of #1680.

I did notice that some functionality did change during the renaming of @extraTag to @goTag. e.g. if the value argument is not passed the value of the tag is set to the field name. e.g. testField String @goTag( key:"yaml") sets the tag json:"testField" yaml:"testField". Though this was not mentioned in the PR, I assumed that this is the intended behaviour and this has also been implemented in the FieldMutateHook.

I have:

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

This change does not change the logic of goTag, merely reimplements it using a FieldMutateHook and sets it as the default FieldMutateHook for the modelgen plugin.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 54.058% when pulling d1c69eb on tprebs:master into 37a4e7e on 99designs:master.

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.

Thanks! This looks good to me, but I'm also wondering about fleshing out the hooks documentation a bit to promote this technique a bit more.

Copy link
Contributor

@wilhelmeek wilhelmeek left a comment

Choose a reason for hiding this comment

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

Though this was not mentioned in the PR, I assumed that this is the intended behaviour and this has also been implemented in the FieldMutateHook.

Yes it was intended. Apologies for not mentioning in the PR, but felt that that was a sensible fallback.

@wilhelmeek
Copy link
Contributor

Thanks for putting this together @tprebs! 🙏

@StevenACoffman StevenACoffman merged commit 59a3091 into 99designs:master Nov 1, 2021
@tprebs
Copy link
Contributor Author

tprebs commented Nov 1, 2021

Thanks! This looks good to me, but I'm also wondering about fleshing out the hooks documentation a bit to promote this technique a bit more.

I agree. Though there are some examples in the recipes section, the plugins documentation in the reference section has not been updated in a couple of years

@StevenACoffman
Copy link
Collaborator

I didn't want to hold up merging this PR so I merged it as it was, but I'm sorry if you weren't finished. If you get the time to polish any documentation or examples up more, I would be very grateful and glad to merge them as a separate PR.

@tprebs
Copy link
Contributor Author

tprebs commented Nov 2, 2021

I'm Happy to create another PR updating the plugin reference documentation and hooks. The thoughts I had were

  1. Show the goTag implementation as a hook in the recipes, replacing the current constraint validation example
  2. Add ModelMutateHook and FieldMutateHook documentation to the plugin reference documentation, explaining the scope of what each is used for and what it has access to giving reference to the example implementation which is goTag.
  3. Outline the modelgen hooks lifecycle and when the hooks are applied

Is there anything else you can think of that we should add?

@syssam
Copy link

syssam commented Nov 23, 2021

any update?

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.

5 participants