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

Errors case is not uniform #12031

Open
I3oris opened this issue Apr 26, 2022 · 5 comments
Open

Errors case is not uniform #12031

I3oris opened this issue Apr 26, 2022 · 5 comments

Comments

@I3oris
Copy link
Contributor

I3oris commented Apr 26, 2022

Hello crystal team,

Most of compiler errors doesn't start with a capital letter after the Error:, for example, following snippets give:

(()

=> Error: unterminated parenthesized expression

foo(

=> Error: unterminated call

[

=> Error: unterminated array literal

{"foo",

=> Error: unterminated tuple literal

{"foo" => "bar",

=> Error: unterminated hash literal

macro foo

=> Error: unterminated macro

But some others, give errors with a capital letter:

`foo

=> Error: Unterminated command literal

"foo

=> Error: Unterminated string literal

"#{foo

=> Error: Unterminated string interpolation

There are also few others with capital letter like:

<<-FOO

=> Error: Unexpected EOF on heredoc identifier

It's really a small detail, but I think it's better if errors are uniform, for some kind of consistency.

I have noticed that because in my repo IC, the Interpreter UI should test again the unterminated errors to know if it should go multiline or not.

So, does the norm is well to keep lowercase, or would it be better to finally always put a capital ?

NOTE: I have also remarked the same for "BUG" errors, which are most of the time in capital, but sometime in titlecase: "Bug".
NOTE: I haven't show all occurrences here, but I could come with a PR with all modified occurrence inside.

@beta-ziliani
Copy link
Member

I'm already surprised that there is a consistency about the different errors to start with the word "unterminated". In any case, while I slightly prefer starting with lowercase after Error: , probably is best/easier to just statistically change the less common into the most common.

@I3oris
Copy link
Contributor Author

I3oris commented Apr 26, 2022

Yeah, there are actually plenty of "unexpected token", or "expecting token", but that rather uniform.

I prefer lowercase too, but I realize that the only problem is it's makes harder to see the difference between:

private def unexpected_token
  raise "Unexpected token: #{token}"
end

which should take an upper letter, because is located in API json pull_parser, and doesn't starts with "Error:"

and;

def unexpected_token(msg : String? = nil, token : Token = @token)
    token_str = token.type.eof? ? "EOF" : token.to_s.inspect
    if msg
      raise "unexpected token: #{token_str} (#{msg})", @token
    else
      raise "unexpected token: #{token_str}", @token
    end
end

located in syntax parser, but call raise defined in the Parser object, which will add the "Error:" at beginning.

@straight-shoota
Copy link
Member

straight-shoota commented May 23, 2022

Sorry for jumping in late.

I'm not sure about starting error messages with a lower case character. Error messages are not entire sentences, but a similar structure. IMO it makes some sense to start with an upper case character. I'm not fully commited to this. I think most programming languages use lower-case error messages.

But I think there should be no difference between casing in messages of runtime errors and compile time errors. I see them as fundamentaly the same thing. Compile time errors are just runtime errors in the compiler. And why should we treat rutime error messages in the compiler differently from runtime error messages in the standard library?

The Error: prefix added by the compiler is just a UI component and not part of the actual error message. The error message should work in other contexts without the Error: prefix. So this shouldn't really affect the decision for upper or lower case.

@I3oris
Copy link
Contributor Author

I3oris commented May 30, 2022

Hello, sorry too for answering late.

That makes totally sense! I think the most of shards starts their exceptions with an upper. I does intuitively the same.

In this case, feel free to ignore my PR. If we decide so, I could propose the upper-case version. However, I understand that absolutely not the priority, so It not urgent to decide or merge.

@straight-shoota
Copy link
Member

Update: #13400 adapted all stdlib exceptions to sentence case. This was already mostly the case, this affected only some outliers.

I would suggest to follow up for compiler messages and make them all use sentence case as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants