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

introduce ast.Path type #117

Merged
merged 5 commits into from
Feb 9, 2020
Merged

introduce ast.Path type #117

merged 5 commits into from
Feb 9, 2020

Conversation

vvakame
Copy link
Collaborator

@vvakame vvakame commented Feb 7, 2020

this is a Breaking Change.
but I think it's better to do it before gqlgen becomes 1.0.0.

Motivation.
[]interface{} is unclear typing.
I read source code that what's contains in Path values often.
and, I want to use string format path value.
It uses for collect gqlgen metrics & logs to OpenCensus / Stackdriver Logging.

@vvakame vvakame requested a review from vektah February 7, 2020 07:18
case PathIndex:
str.WriteString(fmt.Sprintf("[%d]", v))
case PathName:
if i != 0 {
Copy link
Owner

@vektah vektah Feb 7, 2020

Choose a reason for hiding this comment

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

bug if path starts with number?

eg .[0].foo.bar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

expected is [0].foo.bar ? 🤔
I think number is not coming at head of path.
so, this PR can't validate element value.

  1. don't place number at head
  2. don't contains nil value

Copy link
Owner

@vektah vektah left a comment

Choose a reason for hiding this comment

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

Textbook use case for Discriminated Unions :(

LGTM

Copy link
Owner

@vektah vektah left a comment

Choose a reason for hiding this comment

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

Actually, can we check how bad the bc break is here?

Will the old code fail at runtime only? If so I dont think we should ship this

@coveralls
Copy link

coveralls commented Feb 9, 2020

Coverage Status

Coverage decreased (-0.2%) to 92.143% when pulling fcdc574 on feat-path into c06d8e0 on master.

vvakame added a commit to 99designs/gqlgen that referenced this pull request Feb 9, 2020
@vvakame
Copy link
Collaborator Author

vvakame commented Feb 9, 2020

@vektah 99designs/gqlgen#1022 I tried to apply this patch to gqlgen.
BC is maybe small.. (I think)

Will the old code fail at runtime only?

New code is developer-friendly. prevention of runtime errors may be small 🤔

@vvakame vvakame requested a review from vektah February 9, 2020 09:22
vvakame added a commit to 99designs/gqlgen that referenced this pull request Feb 9, 2020
Copy link
Owner

@vektah vektah left a comment

Choose a reason for hiding this comment

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

Yeah cool, its a compile time break. 👍
image

@vvakame vvakame merged commit a7a59ec into master Feb 9, 2020
@vvakame vvakame deleted the feat-path branch February 9, 2020 10:53
@vvakame
Copy link
Collaborator Author

vvakame commented Feb 9, 2020

@vektah thanks! please ping me when add new tags for this repo. I'll update gqlgen side.

vektah added a commit to 99designs/gqlgen that referenced this pull request Feb 16, 2020
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
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