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

Several improvements incl GraphQL option & addtl called detection #48

Closed

Conversation

TSMMark
Copy link

@TSMMark TSMMark commented Dec 20, 2022

Adds a bunch of extra called detection which should benefit all users of this gem.


Also adds an option --graphql to handle the graphql-ruby behavior:

field :name, String, null: true

the default resolution behavior for a field called name is to invoke the name method on the object.

@TSMMark TSMMark changed the title implement GraphQL field method_name called Several improvements incl GraphQL plugin & addtl called detection Dec 21, 2022
@TSMMark TSMMark changed the title Several improvements incl GraphQL plugin & addtl called detection Several improvements incl GraphQL option & addtl called detection Dec 23, 2022
@TSMMark
Copy link
Author

TSMMark commented Jan 6, 2023

I believe most of these changes are general improvements that should serve all users of this gem and ideally this PR should be merged reasonably as soon as possible.

Anything I need to change before a merge would be accepted?

@zenspider zenspider self-assigned this Jan 24, 2023
@zenspider
Copy link
Member

I'm poking at this...

@zenspider
Copy link
Member

I'm not convinced that the graphql part "fits"... I'm trying to think up a means to make debride more configurable, but even with sexp_processor's pattern matching, that's hard to do flexibly...

@TSMMark
Copy link
Author

TSMMark commented Feb 13, 2023

I'm not convinced that the graphql part "fits"... I'm trying to think up a means to make debride more configurable, but even with sexp_processor's pattern matching, that's hard to do flexibly...

For sure. I originally wanted to make debride more extensible with custom matchers and things but that ended up being more complicated. Instead I went with the simpler approach which was to follow the existing pattern for the rails flag, which adds some matchers when used in a Rails env. The GraphQL flag does the same, but for a graphql env.


I do think making the library more open to extension / plugins would be ideal though.

I really like Jeremy Evans' plugin pattern, described here https://janko.io/the-plugin-system-of-sequel-and-roda/

next unless Sexp === block
next unless block.sexp_type == :lit
called << block.last
end
Copy link
Member

Choose a reason for hiding this comment

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

why not def process_block_pass?

Copy link
Member

Choose a reason for hiding this comment

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

damnit... apparently I wrote this a long time ago... never submitted I guess. I've moved this and a bit more over to their own processor methods.


GRAPHQL_OBJECT_METHODS = [
:field,
]
Copy link
Member

Choose a reason for hiding this comment

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

I'm punting on this stuff for now so I can get SOMETHING out

@zenspider
Copy link
Member

OK. I've added 80+% of this w/ some refactoring / moving to process_X calls. It'll go out in the next release. I'm gonna close this, but if you could please open a ticket referencing this one to add some sort of extension system. I'd love to extract the rails code out and the graphql stuff makes sense elsewhere

@zenspider zenspider closed this Mar 21, 2023
@TSMMark
Copy link
Author

TSMMark commented Mar 21, 2023

Cool. Will open a new issue about extension system. In the future if it's not too much to ask, please consider using commits from contributors instead of fully redoing the work. Many people enjoy getting that contributor badge & status. Thanks!

@seattlerb seattlerb locked and limited conversation to collaborators Oct 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants