fix path_string crash when path includes list #1338
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
in Absinthe.Resolution; also: * add documentation for path_string/1 * add specs for path/1 and path_string/1 this fix is absinthe-graphql#1337 absinthe-graphql#1337
benwilson512
requested changes
Sep 2, 2024
| @@ -128,6 +128,7 @@ defmodule Absinthe.Resolution do | |||
| In this case `5` is the 0 based index in the list of users the field is currently | |||
| at. | |||
| """ | |||
| @spec path(t()) :: [String.t()] | |||
| def path(%{path: path}) do | |||
There was a problem hiding this comment.
I don't think this spec is correct. This returns the query nodes, not strings. Also, in the case of lists, it returns integers.
There was a problem hiding this comment.
I'll switch to [String.t() | integer()]
lib/absinthe/resolution.ex
Outdated
|
|
||
| ```elixir | ||
| resolve fn _, _, resolution -> | ||
| Absinthe.Resolution.path(resolution) #=> ["email", "users", "RootQueryType"] |
There was a problem hiding this comment.
The desired output here is probably to stringify the integer so that the information isn't lost. This is also a breaking change so we'll need to release this under a new minor version.
There was a problem hiding this comment.
I've added back the integers as strings. I would suggest this isn't a breaking change, b/c no one was using the previous behavior b/c it was broken.
fix the type on Resolution.path/1 incorporating PR feedback from @benwilson512 issue absinthe-graphql#1337
|
I've tried to address the feedback |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
in Absinthe.Resolution; also:
this fix is #1337