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

Add location to buildMessage #608

Closed
wants to merge 1 commit into from
Closed

Add location to buildMessage #608

wants to merge 1 commit into from

Conversation

vetedde
Copy link

@vetedde vetedde commented Apr 14, 2019

It helps create usefull message with postioin of error

PR type

Add new parameter location to buildMessage function

Prerequisites

To make the bug messages more readable I needed to know their location. For this, I pass as an additional parameter location to buildMessage function/

  • I have read the CONTRIBUTING.md document:
  • I have updated the documentation accordingly:
  • I have added tests to cover my changes:

Description

P.S. It is my first pull request, I tried to do everything right, but if I made a mistake, I’m ready to fix it if this pull seems good, but unfinished

It helps create usefull message with postioin of error
@vercel
Copy link

vercel bot commented Apr 14, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://pegjs-next-website-git-fork-vetedde-master.futagoza.now.sh

@Mingun
Copy link
Contributor

Mingun commented Apr 14, 2019

You add a new parameter but do not use it?..

@vetedde
Copy link
Author

vetedde commented Apr 14, 2019

You add a new parameter but do not use it?..

No, I use it, when I customize my error message

'''
SyntaxError.buildMessage = function(expected, found, location) {
return Excpected ${describeExpected(expected)}, but found ${found} at ${ location.start.line };
}
'''

@Mingun
Copy link
Contributor

Mingun commented Apr 14, 2019

There are some reasons for which it is necessary to replace buildMessage to change the message? In thrown exception you have all information to make any message what you want

@vetedde
Copy link
Author

vetedde commented Apr 15, 2019

There is a built-in ability to customize the message that will be displayed to the user with help buildMessage. I don't see the point of writing error handling to display the user the same message, but with the location, so I just added a location parameter

@futagoza
Copy link
Member

futagoza commented Apr 15, 2019

@Mingun I think this is fine, as some people don't like using try...catch, and overriding the default behaviour of this function gives them the ability to customise the error messages without using try...catch. This PR simply gives them the location as well.

@vetedde I'm fine with merging this once you:

  • There's no need to commit these files because of a removed empty line:
    • packages/pegjs/lib/peg.js
    • packages/pegjs/bin/peg.js
  • Remove all package-lock.json files
  • Don't edit website/vendor/pegjs/peg.js, thats PEG.js v0.10

@futagoza futagoza closed this in 1159660 Sep 29, 2019
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