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

Provide a more robust error message if JSON is invalid. #177

Merged
merged 2 commits into from
Apr 3, 2015

Conversation

Tonkpils
Copy link
Contributor

@Tonkpils Tonkpils commented Apr 2, 2015

Fixes #170

With json.SyntaxError we also have the Offset at which the error is, so we can highlight where the syntax error is. I can work on that if that's something we want but this should be enough to get going.

@Tonkpils
Copy link
Contributor Author

Tonkpils commented Apr 2, 2015

Current output looks like this:

error parsing JSON in the config file ~/config_invalid.json:
The file contains invalid JSON syntax: invalid character 'e' in string escape code

Here's an example output of how it would look if we were to highlight the syntax error

error parsing JSON in the config file ~/config_invalid.json:
The file contains invalid JSON syntax at ':"D:\\Users\e' <~
invalid character 'e' in string escape code

Thoughts? Not sure if it adds any more value.

@mtakac2
Copy link

mtakac2 commented Apr 2, 2015

Would it be possible to add full path to the config and number of line causing the error?

@jwood803
Copy link

jwood803 commented Apr 2, 2015

Each bit more of info you can supply to folks always helps, I say. :]

@Tonkpils
Copy link
Contributor Author

Tonkpils commented Apr 2, 2015

Would it be possible to add full path to the config and number of line causing the error?

@mtakac The full path will be displayed, I changed it for the example but it will look like

error parsing JSON in the config file /var/folders/_2/0wpr12717gg2g8btfc3fvr4w0000gn/T/485490851/config_invalid.json

Adding the line number should be trivial but the issue is that the config file is usually just 1 line unless the user modifies it to be pretty.

Each bit more of info you can supply to folks always helps, I say. :]

Definitely! Though I wanted to get feedback in case too much info can be overwhelming for the user but I guess it couldn't hurt in this scenario since they're already running into an issue.

@mtakac2
Copy link

mtakac2 commented Apr 2, 2015

👍 looks great and clear.

@Tonkpils
Copy link
Contributor Author

Tonkpils commented Apr 2, 2015

The latest commit makes the error output:

2015/04/02 19:42:21 error parsing JSON in the config file /Users/tonkpils/.exercism.json:
The file contains invalid JSON syntax at 'm.io", "foo"}' <~
invalid character '}' after object key

@kytrinyx
Copy link
Member

kytrinyx commented Apr 3, 2015

We could also write the config file pretty--it's not costly for such a small file, and if it would make it easier to give good error messages, that's a plus.

@Tonkpils
Copy link
Contributor Author

Tonkpils commented Apr 3, 2015

@kytrinyx sure, do you want me to get that in with this PR or in a different one?

@kytrinyx
Copy link
Member

kytrinyx commented Apr 3, 2015

pretty can happen in a separate PR, I was just thinking out loud :)

@kytrinyx
Copy link
Member

kytrinyx commented Apr 3, 2015

Thanks @Tonkpils!

kytrinyx added a commit that referenced this pull request Apr 3, 2015
Provide a more robust error message if JSON is invalid.
@kytrinyx kytrinyx merged commit 95440f2 into exercism:master Apr 3, 2015
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.

Provide a useful error message if the config JSON is invalid
4 participants