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

eval_result_parser bugs #21

Closed
db4 opened this issue Jul 6, 2016 · 11 comments
Closed

eval_result_parser bugs #21

db4 opened this issue Jul 6, 2016 · 11 comments

Comments

@db4
Copy link
Contributor

db4 commented Jul 6, 2016

  • print output may contain \n. The fix:
-_ = $([ \t]+) {}
+_ = $([ \t\n]+) {}
  • OCaml identifiers may contain digits. The fix:
-id = $([A-Za-z_'.]+)
+id = $([A-Za-z_][A-Za-z_'.0-9]+)

BTW, can you handle parsing errors better than

        try {
            let data = evalResultParser.parse(text);
            return createVariable(data.name, data.value);
        } catch (ex) {
            return null;
        }

so one could know that an error has been occurred? Maybe to show non-parsed value with an error indicator? It's especially useful when someone installs a pretty printer that does not conform to your parsing rules.

hackwaly added a commit that referenced this issue Jul 6, 2016
@hackwaly
Copy link
Owner

hackwaly commented Jul 6, 2016

Try 0.4.14. You can use "_showLogs": true.

@db4
Copy link
Contributor Author

db4 commented Jul 7, 2016

I've found another bug: the parser fails on nested tuples:

test : int * (int * int) = (1, (2, 3))

And can you improve logging so that the error location could be seen? Something like that:

        } catch (ex) {
            let start = ex.location.start.offset;
            let end = Math.max(ex.location.end.offset, Math.min(start + 16, text.length));
            let location = text.substring(start, end);
            this.log(`Error (${ex}) occurs while parsing "${location}...".`);
            return null;
        }

@hackwaly
Copy link
Owner

hackwaly commented Jul 7, 2016

Since VS marketplace do not have beta test support. It bothers other users every time I publish a unstable version. I'd like to give you a packaged extension or you can make it from git repo.

npm install -g vsce
cd vscode-ocaml
vsce package

You can drag produced .vsix file to vscode to install it.

I'm now going to fix the parsing error. And I'll notify you when a test version pushed to master branch on github repo. Then you can pull it and test that.

@db4
Copy link
Contributor Author

db4 commented Jul 7, 2016

Github repo is ok, thanks. I'm able to build the extension locally.

hackwaly added a commit that referenced this issue Jul 7, 2016
@hackwaly
Copy link
Owner

hackwaly commented Jul 7, 2016

Have a try! If you can't build .vsix file, I can upload it to github repo.

@db4
Copy link
Contributor Author

db4 commented Jul 7, 2016

.vsix is built OK, so I've found another bug. I'll create PR shortly.

@hackwaly
Copy link
Owner

hackwaly commented Jul 7, 2016

I haven't wrote a build script that auto compile .pegjs to .js. You can compile it use pegjs online editor http://pegjs.org/online. The build script and test CI are planned.

@db4
Copy link
Contributor Author

db4 commented Jul 7, 2016

I have no problem in compiling .pegjs locally.

@hackwaly
Copy link
Owner

hackwaly commented Jul 8, 2016

Does there any more bugs found? Is it a good time to publish a new bug fix version?

@db4
Copy link
Contributor Author

db4 commented Jul 8, 2016

For me it works as expected. There are no showstoppers. I would also love to see

Show Variable Type on Hover
If a debug extension supports variable types, we now show the type when hovering over the variable name.

feature of VS Code 1.3.0 supported but of course it can be made later.

@hackwaly
Copy link
Owner

hackwaly commented Jul 8, 2016

OK, wait for that. microsoft/vscode#8040

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

No branches or pull requests

2 participants