Fixes tests that was broken with enhancements made to the `lexer` #2

sogko · 2016-06-12T11:52:33Z

The changes here includes changes in PR #1 (which updatesmatthias247/graphql:lexperf with latest changes from graphql-go/graphql:master). Merge that first before reviewing the changes here. 👍

Fixes tests that was broken with enhancements made to the lexer with commit be39d41

Running go test would have passed previously, but running all tests with go test ./... would fail.
Increased coverage for lexer package to 100%
Added more tests to cover parsing unicode strings as well. (see Error for utf8 symbols in arguments graphql-go/graphql#135)

This improvements made to the lexer in this branch do improve the perf by quite a bit (See separate discussion at sogko#22 (comment)). Would be glad to add it to v0.5.0 release.

Cheers!

@include

* master: (32 commits) RFC: Return type overlap validation Tests for `NewDirectives()` for better coverage Clean up redundant code in `completeValueCatchingError` - `completeValue` would already have resolved the value for `func() interface{}` Add tests for type comparators Spec compliant @skip/@include. Fix bug where @include directive is ignored if @Skip is present. Test that `executor` threads `RootValue` correctly - Similar to `context` test Improve coercion error messages [RFC] Add explicit context arg to graphql execution Add GraphQLSchema types field Removed logs Follow up to: Move getTypeOf to executor and rename to defaultResolveTypeFn to mirror defaultResolveFn Add tests for default resolve function. Restored deprecated fields in `directives` in introspective query for coverage. Should be removed once `onOperation`, `onFragment`, `onField` are dropped from spec. Move getTypeOf to execute.js and rename to defaultResolveTypeFn to mirror defaultResolveFn Remove unused function parameters Remove unused function parameters Minor follow-up to graphql-go#311 [RFC] Add Schema Definition to IDL. Updating schema parser to more closely match current state of RFC ...

commit Matthias247@be39d41 - Running `go test` would have passed previously, but running all tests with `go test ./...` would fail. - Increased coverage for `lexer` package to 100% - Added more tests to cover parsing unicode strings (see graphql-go#135) as well.

Matthias247 · 2016-06-12T11:59:26Z

Hi,
sorry, I don't understand the intent of these pull requests? Can't you directly merge my PR into the main go-graphql repo? Or merge my commit into one of your PRs? I only forked this repo temporarily in order to provide the pull request. And unfortunatly I'm currently not able to review all other changes.

sogko · 2016-06-12T12:07:42Z

Hi @Matthias247

Oops, let me explain :)

Your lexperf branch got outdated (graphql-go#137) after the main repo merged huge changes from graphql-go#123.

The only relevant commit in this PR is 2762599 (I've fixed the lexer tests after your improvements to the lexer and fixed some issues with your original implementation).

The other commits would be gone after you merge in PR #1 (which will bring your lexperf branch up-to-date, you don't have to review those in #1 if you don't want to :) )

After both PR #1 and #2 has been merged, I can close your original PR graphql-graphql-go#137

Hope that clears it up a little bit?

Cheers!

Matthias247 · 2016-06-12T12:11:39Z

Ah, I see it now. But wouldn't it make much more sense to rebase the PR on current graphql-go/master and merge it then? I can do that. Merging the things here and merging the merged product back in the original repo sounds like duplicated merge of your commits.

Matthias247 · 2016-06-12T12:14:47Z

As an alternative you could also cherry pick my single lexer commit.

sogko · 2016-06-12T12:15:24Z

Yeah thats the ideal way, please do that :)
I was just got too excited with bringing in your changes in and getting the tests fixed.

Matthias247 · 2016-06-12T12:29:58Z

@sogko Rebased my PR on top of the current master branch. However that no misses the latest changes to the lexer that you made in the last commit here (e.g. fixing tests).

Matthias247 · 2016-06-12T12:36:26Z

language/lexer/lexer.go

-		if (end != bodyLength) &&
+		code, n := runeAt(body, endByte)
+		if (endByte != bodyLength) &&
+			n == 1 && // only ASCII chars are allowed to be names


The n == 1 check is superfluos here, since the next and condition only checks for char codes which are all single-byte characters.

Yeah, I agree, I'll remove that 👍

Matthias247 · 2016-06-12T12:49:03Z

I have seen that you added functionality to also keep the current runeIndex in memory. I think that's ok. But I don't know if passing that to NewSyntaxError is the correct thing. If I understand the method and GetLocation correctly they are operating on bytes (the lineRegexp does). If showing the correct column (as a rune and not byte index) inside the line is imporant then I think the amount of runes up to position can be counted for the line inside the GetLocation function.

sogko · 2016-06-12T13:51:39Z

Yeah, I'm using the runePosition for SyntaxError, which gives out a nicely formatted error and it highlights the position of rune at which the syntax error occurs. (Since readString() can contain unicode text)

It certainly has limitations at the moment (for e.g. runes that are 3- and 4-bytes wide may not sit in one column, resulting the ^ "pointer" being off.)

I didn't spend much time on that, it certainly would be nice to see this getting tackled. Do you want to address that in a separate PR? It would be related to ensuring a nice coverage over unicode support. (Current test suite are lacking that at the moment)

…ing type - Removed unnecessary check of length of run in `readName()`

Matthias247 · 2016-06-12T14:32:54Z

I think the most reasonable way to do this is to track the current line as well as in the Lexer to avoid the GetLocation call alltogether. The current implementation of GetLocation is quite costly, as it allocates a new string for each line. It could be replaced by a linear scan through the body to scan for line breaks and count columns. But as this is basically already done during lexing it would be redundant, especially if there are multiple errors.

So the lexer would need to track multiple variables:

Current byte offset
Current line nr
Current column nr

Sorry - don't have too much time to look into this now. For me the most imporant thing was to optimize the "happy path".

Matthias247 · 2016-06-12T14:35:32Z

So how should be proceed with this? Was your plan that I merge this and you merge the result back into graphql repo? If yes then this is PR refers the wrong branch, because my rebased changes are in the lexperf branch and not on the master branch. Otherwise you can simply merge my PR to the main graphql repo. And then merge your changes on top of it.

sogko · 2016-06-12T14:38:42Z

Haha sure no problem, I'll put the improvement for GetLocation() in the backlog. Right now we are focused on getting it right and relying on the test suites to ensure that we are on the right track.

I'll merge your PR to the main repo and push the updates on top of it 👌

Thanks for contributing your time on this, appreciate it very much!

Cheers!

Matthias Einwag and others added 3 commits June 10, 2016 15:08

Improve lexer performance by using a byte array as source

be39d41

Matthias247 force-pushed the master branch from be39d41 to 500a3ff Compare June 12, 2016 12:25

Matthias247 reviewed Jun 12, 2016
View reviewed changes

Fixed invalid test for lexer properly escaping slashes for TokenStr…

69d5dba

…ing type - Removed unnecessary check of length of run in `readName()`

sogko mentioned this pull request Jun 12, 2016

Fix lexer tests (follow up to #137) graphql-go/graphql#138

Merged

sogko closed this Jun 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixes tests that was broken with enhancements made to the `lexer` #2

Fixes tests that was broken with enhancements made to the `lexer` #2

Uh oh!

sogko commented Jun 12, 2016

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

sogko commented Jun 12, 2016

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

sogko commented Jun 12, 2016

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

Matthias247 Jun 12, 2016

Uh oh!

sogko Jun 12, 2016

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

sogko commented Jun 12, 2016

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

sogko commented Jun 12, 2016

Uh oh!

Uh oh!

Fixes tests that was broken with enhancements made to the lexer #2

Fixes tests that was broken with enhancements made to the lexer #2

Uh oh!

Conversation

sogko commented Jun 12, 2016

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

sogko commented Jun 12, 2016

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

sogko commented Jun 12, 2016

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

Matthias247 Jun 12, 2016

Choose a reason for hiding this comment

Uh oh!

sogko Jun 12, 2016

Choose a reason for hiding this comment

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

sogko commented Jun 12, 2016

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

Matthias247 commented Jun 12, 2016

Uh oh!

sogko commented Jun 12, 2016

Uh oh!

Uh oh!

Fixes tests that was broken with enhancements made to the `lexer` #2

Fixes tests that was broken with enhancements made to the `lexer` #2