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

Improve DX #33

Closed
wants to merge 4 commits into from
Closed

Improve DX #33

wants to merge 4 commits into from

Conversation

oxyc
Copy link
Collaborator

@oxyc oxyc commented Apr 25, 2016

Considering adding this as v3. Seems more logical to set the humanly readable line numbers in the cli instead of hardcoding into the message.

@mathiasbynens
Copy link
Contributor

Is this cross-browser-compatible?

@oxyc
Copy link
Collaborator Author

oxyc commented Apr 25, 2016

The only real change is that context data are now solely attached to the error object, and can't be gleaned from the message string. I've never researched but I assume thrown objects work with properties in all legacy browsers?

The main reason for this change is that I've noticed all libraries that use luaparse need to manipulate the error object to be usable, ie https://github.com/rameshvarun/linter-luaparse/blob/master/lib/linter.js#L17:L20.

If we go this way, it would also be nice to get at least the plumbing for tolerant error handling before bumping the major version.

@mathiasbynens
Copy link
Contributor

mathiasbynens commented Apr 25, 2016

I was asking because error objects have historically had different names and formats for properties/values in different engines. Not sure what the current status is on name, line, column, and message.

@oxyc
Copy link
Collaborator Author

oxyc commented Apr 25, 2016

Ah, thanks for the heads up! I'll make sure to look into it.

@fstirlitz
Copy link
Owner

@oxyc, are you planning to merge this at some point? May want to rebase it with my changes to the testing framework.

@oxyc
Copy link
Collaborator Author

oxyc commented Feb 17, 2017

@fstirlitz thanks for the heads up, I'll do that. As this is a breaking change which doesn't really fix any bugs or add much, I'm in no hurry to get it merged. I was thinking if there's ever some larger feature (eg error tolerance), it could be added in the same version bump.

Lately seems I've been neglecting the project though, thanks for keeping it going!

@fstirlitz
Copy link
Owner

As for breaking changes, I've already added the Lua versioning switch and interpretation of escape sequences in string literals. What do you think about the issue I mentioned in a comment in readUnicodeEscapeSequence(), by the way?

And with regards to this specific change, I'd also change the error object so that it's no longer a SyntaxError. After all, SyntaxError is meant for errors in JavaScript code. How do you distinguish a syntax error in luaparse from an error in the input fed to it?

@fstirlitz
Copy link
Owner

@oxyc: w.r.t. SyntaxError, please take a look at 8536fdf.

@fstirlitz fstirlitz added compat hazard Resolving this issue may create backwards compatibility problems enhancement Request for functionality covering an entirely new use case labels Aug 2, 2019
@fstirlitz fstirlitz force-pushed the master branch 3 times, most recently from 8022c30 to 365842c Compare October 24, 2019 10:36
@fstirlitz
Copy link
Owner

This PR has stalled, and I think I'll be addressing problems raised here in a different way. Closing.

@fstirlitz fstirlitz closed this Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compat hazard Resolving this issue may create backwards compatibility problems enhancement Request for functionality covering an entirely new use case
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants