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: allow imports from same generated package #289

Closed
wants to merge 1 commit into from

Conversation

avinassh
Copy link

@avinassh avinassh commented Jul 28, 2023

This is a small change, but it lets you import marshaler and unmarshalers from the same generated package.

Why?
I'd like to keep all the graphql related code into one. Following the conventions of the genqlient/example, I would like to keep a custom marshaler in the same directory as example. For e.g., example/helper.go could contain:

// helper.go
func UnMarshalDateTime(b []byte, v *time.Time) error {
	...
	return nil
}

But following won't work:

...
generated: generated.go
package: main
bindings:
  DateTime:
    type: time.Time
    unmarshaler: "https://github.com/Khan/genqlient/main.UnMarshalDateTime" // since package name is main
    # unmarshaler: "https://github.com/Khan/genqlient/example.UnMarshalDateTime" // nor this, if one is using a custom package

So, instead I would like to add a new feature where one can import from the same package:

...
generated: generated.go
package: main
bindings:
  DateTime:
    type: time.Time
    unmarshaler: "main.UnMarshalDateTime"

What do you think? If you think this is good, then I will add tests and update the documentation

I have:

  • Written a clear PR title and description (above)
  • Signed the Khan Academy CLA
  • Added tests covering my changes, if applicable
  • Included a link to the issue fixed, if applicable
  • Included documentation, for new features
  • Added an entry to the changelog

Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request!

Did you see #285? I think you're both trying to solve the same issue. I like the narrower approach here -- as I said on that PR I think if we introduce new syntax we need to make sure we get it right whereas you sidestep that. But I don't think what you have quite works as-is, because the package option is the package name, not the full package path.

Relatedly, a test would help show whether it works! And is definitely necessary -- along with a changelog entry -- to merge.

@StevenACoffman StevenACoffman removed their request for review October 12, 2023 18:39
@benjaminjkraft
Copy link
Collaborator

Closing in favor of #316,

@avinassh avinassh deleted the same-pkg branch February 18, 2024 11:36
@avinassh
Copy link
Author

Hey @benjaminjkraft I missed the previous notification. I checked #316 and I think its in the right direction 👍

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.

2 participants