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

feat: Add new plugin signatures #2011

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

cliedeman
Copy link
Contributor

Describe your PR and link to any relevant issues.

See #1994 #1995

I have:

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

@cliedeman
Copy link
Contributor Author

@StevenACoffman any feedback?

@cliedeman cliedeman closed this Jun 16, 2024
@StevenACoffman
Copy link
Collaborator

Oh wow, I somehow overlooked this completely. I'm sorry!

@StevenACoffman StevenACoffman marked this pull request as ready for review June 17, 2024 11:47
@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Jun 17, 2024

I'm pretty hesitant to break backwards compatibility, and at the time this was first proposed, it was extremely important to regain the confidence of the community since there had been a period of instability.

I also agree forcing panic-recover as an error-passing mechanism is definitely gross. The pugins that are part of this repository are pretty easy to update, and all the Ent folks are quite responsive.

gqlgen Plugins

@cliedeman
Copy link
Contributor Author

I did a quick check and the only plugins that use InjectSources are autogql, ent and federation.

Also it shouldn't be a breaking change. Ielft the previous interface intact

@cliedeman cliedeman changed the title feat: Change plugin signatures feat: Add new plugin signatures Jun 17, 2024
@StevenACoffman
Copy link
Collaborator

I can't push to your branch, but I resolved the conflicts and rebased:
a2d3d70

If you can either apply those changes, or resolve the conflicts yourself, I will merge this PR, and again sorry for overlooking it.

@cliedeman
Copy link
Contributor Author

thanks @StevenACoffman
I applied your commit

@coveralls
Copy link

Coverage Status

coverage: 74.666% (-0.06%) from 74.728%
when pulling a2d3d70 on cliedeman:feat/plugins
into 04b13fd on 99designs:master.

@StevenACoffman StevenACoffman merged commit 1422ff2 into 99designs:master Jun 18, 2024
17 checks passed
@StevenACoffman
Copy link
Collaborator

Thanks! I really appreciate your patience and commitment to this improvement.

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.

3 participants