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

Consistently use quotes in parsing errors #4762

Closed
wants to merge 1 commit into from

Conversation

nalimilan
Copy link
Member

Use quotes around identifier and operators, as well as around
parsed expressions to make understanding messages easier.

This is a change I felt the need for after getting a message like

ERROR: unrecognized named argument types

which is much clearer as

ERROR: unrecognized named argument "types"

This can be even more useful when a weird user-typed expression that could not be parsed is printed.

I've reused the convention to use double quotes, which I found in one string. Another string used single quotes around :. If you prefer single quotes in some cases, and double quotes in other, I can adapt the commit. But I find such a convention very hard to respect in a large code base, let alone package authors.

I'm not familial at all with Scheme, so I hope I've not broken anything. Most cases are simple, but you should probably have a look at the lines I changed that contain #\. and ': (i.e. line 206 and line 534).

@JeffBezanson
Copy link
Member

string simply converts all of its arguments to a string. In both those cases #\. and ': were intended to be part of the printed input text, i.e. should be inside the quotes.

@nalimilan
Copy link
Member Author

OK, so ': is the way to escape : in Scheme? If it is, then I get it.

But as regards #\., I still don't see why it is added to the end of the string, and I could find a way to trigger the error. But that does not really matter, if you're happy with the new version, so am I! ;-)

@JeffBezanson
Copy link
Member

Because the error in that case is that there are too many dots, and the extra dot has not been added to the output string yet. It should say

julia> 2.2.
ERROR: syntax: invalid numeric constant "2.2."

not

julia> 2.2.
ERROR: syntax: invalid numeric constant "2.2".

@nalimilan
Copy link
Member Author

Funny, I had tried everything but not two dots. So my patch works!

@JeffBezanson
Copy link
Member

Please rebase.

Use quotes around identifier and operators, as well as around
parsed expressions to make understanding messages easier. This
commit only deals with src/ files.
@nalimilan
Copy link
Member Author

The last version deals with all files in src/. I think base/ can wait. ;-)

@JeffBezanson
Copy link
Member

Ok, I think this is more changes than I'm comfortable with. It also breaks a test.

For parse errors, quoting makes sense because you are quoting back the user's input text. But I don't think it makes sense in all of these cases. For one thing, quoted strings are also part of the language, so there is confusion about whether you are referring to a string or some other kind of value. Quoting function names every time they're used just looks weird.

@nalimilan
Copy link
Member Author

Sure, there's no hurry, we can discuss the rules you want to apply before merging anything.

Maybe a better rule would be to use double quotes for user-typed expressions (meaning: "you typed that, I cannot be sure what you meant") and single quotes for keywords that have been identified as something in the language (control flow, types, functions...). (I don't think the confusion with String is possible, since error messages rarely, if ever, print String objects as such.)

I've quoted functions and names part of the language because I found some could be confusing in some messages, in particular local, e.g.:

ERROR: syntax: "extra token local after end of expression"

vs

ERROR: syntax: "extra token "local" after end of expression"
(BTW, outer quotes should be removed here...)

That's the case with many several keywords or functions (which, by, in, copy, replace...). In general, I think that any expression that refers to an identifier rather than being plain English would benefit from being highlighted in some way, just like here in Github we use identifier as much as we can. It could even use bold or a lighter font in the terminal instead of single quotes, which would be less heavy to the eye.

Though I agree that messages of the form "function: an error message" do not gain anything by adding quotes around "function" - we could make an exception for them.

@JeffBezanson
Copy link
Member

I'm willing to merge the version that just changes the parse errors.

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.

2 participants