close
The Wayback Machine - https://web.archive.org/web/20200918184326/https://github.com/ethereum/solidity/issues/7578
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

Make usage of quotes in errors consistent #7578

Open
Marenz opened this issue Oct 28, 2019 · 20 comments
Open

Make usage of quotes in errors consistent #7578

Marenz opened this issue Oct 28, 2019 · 20 comments

Comments

@Marenz
Copy link
Contributor

@Marenz Marenz commented Oct 28, 2019

Our error messages are not consistent regarding the use of quotes, e.g. some quote the function name and contract name, others don't.

We should agree on how we want to do that and make it consistent everywhere.

@erak argues for double quotes " around all user defined names.
I tend to agree.

@birtony
Copy link

@birtony birtony commented Oct 29, 2019

Hey, I would be happy to improve the error messages!

@Marenz
Copy link
Contributor Author

@Marenz Marenz commented Oct 29, 2019

Glad to hear that!

We still have to decide on how exactly this should be done, which we probably will do in tomorrows meeting. Feel free to join tomorrow (Wednesday at 15 CET, a link will be posted in gitter) in our google hangout meeting if you want to talk about it.

In any case I'll update this issue with the result of the discussion!

@birtony
Copy link

@birtony birtony commented Oct 29, 2019

Thank you, @Marenz!

@birtony
Copy link

@birtony birtony commented Oct 30, 2019

Hey @Marenz! Unfortunately I could not join you for the meeting today. Have you been able to reach any agreement on this issue?

@Marenz
Copy link
Contributor Author

@Marenz Marenz commented Oct 31, 2019

Yes. We'd like for all user defined names to be in quotes in all error messages. Sorry I didn't update sooner.

@birtony
Copy link

@birtony birtony commented Oct 31, 2019

In single or in double quotes?

@Marenz
Copy link
Contributor Author

@Marenz Marenz commented Nov 4, 2019

Double quotes

@birtony
Copy link

@birtony birtony commented Nov 21, 2019

I was looking into the error messages, and I have also noticed, that a lot of custom defined errors are used. The problem is that I am not totally sure, which exact errors should be changed. Could you, please, provide me with an example of any error that has "user defined names" within it that needs to be updated?

@Marenz
Copy link
Contributor Author

@Marenz Marenz commented Nov 26, 2019

Here are a few examples for you: :)

// Uses single quotes instead of double quotes
m_errorReporter.warning(_statement.location(), "Failure condition of 'send' ignored. Consider using 'transfer' instead.");
// Should be this
m_errorReporter.warning(_statement.location(), "Failure condition of \"send\" ignored. Consider using \"transfer\" instead.");

here no quotes at all are used:

m_errorReporter.typeError(
        _statement.location(),
        errorMsg +
        ". Try converting to type " +
        valueComponentType->mobileType()->toString() +
        " or use an explicit conversion."
);
// Should be this
m_errorReporter.typeError(
        _statement.location(),
        errorMsg +
        ". Try converting to type \"" +
        valueComponentType->mobileType()->toString() +
        "\" or use an explicit conversion."
);


 m_errorReporter.fatalTypeError(_tuple.location(), "Type " + inlineArrayType->toString() + " is only valid in storage.");
// Should be this
 m_errorReporter.fatalTypeError(_tuple.location(), "Type \"" + inlineArrayType->toString() + "\" is only valid in storage.");

@kalashshah11
Copy link

@kalashshah11 kalashshah11 commented Dec 7, 2019

Hey, @Marenz Is this still left to work upon? I would be glad to contribute.

@birtony
Copy link

@birtony birtony commented Dec 7, 2019

@kalashshah11 I am going to push an update in a bit with all the updated error messages I could've found

@axic
Copy link
Member

@axic axic commented Dec 8, 2019

Why not introduce a toQuoted or toQuotedString helper?

@leonardoalt
Copy link
Member

@leonardoalt leonardoalt commented Dec 8, 2019

@axic such as #7733 ? ;)

@axic
Copy link
Member

@axic axic commented Dec 8, 2019

Can't we just implement our alternative in 3 lines of code?

@leonardoalt
Copy link
Member

@leonardoalt leonardoalt commented Dec 8, 2019

Sure, that'd work

@Marenz
Copy link
Contributor Author

@Marenz Marenz commented Dec 9, 2019

When @erak and I talked about it we decided to make it all consistent as a first step and as a second step use a function like that.

@axic
Copy link
Member

@axic axic commented Dec 9, 2019

Sounds like a lot of extra work 😉

@Marenz
Copy link
Contributor Author

@Marenz Marenz commented Dec 9, 2019

I argued along similar lines and I forgot eraks counter-argument ;)

@erak
Copy link
Collaborator

@erak erak commented Dec 9, 2019

Well, I think I had no strong opinion on the process ;-) Would agree with @axic here and start using the helper function right away.

@Marenz
Copy link
Contributor Author

@Marenz Marenz commented Dec 9, 2019

What does that mean in regards to the pending PR?

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

Successfully merging a pull request may close this issue.

6 participants
You can’t perform that action at this time.