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.

Sign up for GitHub

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

Jump to bottom

Added different colors for warning & error #11970

Merged
merged 1 commit into from
Oct 25, 2021
Merged

Added different colors for warning & error #11970

merged 1 commit into from
Oct 25, 2021

Conversation

shikharvashistha
Copy link
Contributor

@shikharvashistha shikharvashistha commented Sep 15, 2021 •
edited
Loading

Fixes : #11962

Greetings of the day @cameel,

I've added different colors for waning and error. Please review the pr and let me know in case of any changes if required.

Thanks & Regards
Shikhar Vashistha

cameel
cameel requested changes Sep 15, 2021
View reviewed changes
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @shikharvashistha! The PR still needs some work. The code does not compile and currently it does not really do what it's supposed to. Please see my comments below.

liblangutil/SourceReferenceFormatter.cpp Outdated

AnsiColorized SourceReferenceFormatter::infoColored() const
{
return AnsiColorized(m_stream, m_colored, {BOLD, YELLOW});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not WHITE? Is that because you think it looks bad or is there some technical obstacle?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation was : In case we've white background then it would create difficulty. But as suggested by you I've changed it to white.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you check how it looks like on white background then? To be more specific, we want to use the default foreground color. I think that terminals with white background will have it remapped to black or something else, but we should verify this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this? Have you checked it how it works in a white terminal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greetings of the day @cameel,

As of now I haven't checked it on white terminal.

Thanks & Regards

cameel
cameel requested changes Sep 17, 2021
View reviewed changes
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's much better now, but still needs some tweaks.

@shikharvashistha shikharvashistha dismissed a stale review via a7bd411 September 20, 2021 02:55
hrkrshnn
hrkrshnn reviewed Sep 20, 2021
View reviewed changes
Copy link
Member

@hrkrshnn hrkrshnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shikharvashistha Could you please remove the formatting changes? It makes the PR rather hard to review now.

@shikharvashistha
Copy link
Contributor Author

@shikharvashistha Could you please remove the formatting changes? It makes the PR rather hard to review now.

Greetings of the day @hrkrshnn ,

I'll remove the formatting. Actually it is caused by using clang-format file present in the source tree.

Thanks & Regards
Shikhar Vashistha

cameel
cameel reviewed Sep 20, 2021
View reviewed changes
cameel
cameel requested changes Sep 23, 2021
View reviewed changes
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rebase it on top of develop? We don't usually merge develop into feature branches. Instead we use rebase workflow.

@shikharvashistha
Copy link
Contributor Author

Hi @cameel, apologies for closing this I've fixed/done the suggested changes

cameel
cameel requested changes Sep 24, 2021
View reviewed changes
cameel
cameel previously requested changes Oct 1, 2021
View reviewed changes
cameel
cameel reviewed Oct 6, 2021
View reviewed changes
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that your code contains syntax errors. They should be pretty obvious if you build the compiler locally. Do you need help doing that?

@cameel
Copy link
Member

cameel commented Oct 7, 2021 •
edited
Loading

As a workaround for the CI not running here I have created a branch in our repo. You can see the results in workflow 19529.

@shikharvashistha
Copy link
Contributor Author

As a workaround for the CI not running here I have created a branch in our repo. You can see the results in workflow 19529.

Thanks for the help extended for all the valuable inputs.

@cameel
Copy link
Member

cameel commented Oct 11, 2021

As you can see, there are lots of CI checks that do not pass. You can see the results by clicking "Details". The PR needs to pass them all to be correct.

@cameel cameel marked this pull request as draft October 11, 2021 19:08
@cameel
Copy link
Member

cameel commented Oct 11, 2021

I marked the PR as draft because it still needs some work. Please use the "Ready for review" button once all PR checks pass. Let me know if you need help with that.

Also, I think it would be more convenient for you to run these tests locally (see Contributing > Running the Compiler Tests).

@shikharvashistha
Copy link
Contributor Author

I marked the PR as draft because it still needs some work. Please use the "Ready for review" button once all PR checks pass. Let me know if you need help with that.

Also, I think it would be more convenient for you to run these tests locally (see Contributing > Running the Compiler Tests).

Okay noted thanks

@cameel
Copy link
Member

cameel commented Oct 22, 2021

You still have lots of syntax and style errors but overall the PR goes in a good direction so to help you out a bit I cleaned it and fixed the errors. Let's see if it passes tests now.

Please take a look at it and see if you'd like to add anything. Then squash commits so that each one is a single logical change (without separate commits for PR fixes).

@cameel cameel marked this pull request as ready for review October 22, 2021 19:15
@cameel cameel dismissed their stale review October 22, 2021 20:21

I have fixed all the problems I pointed out.

@cameel cameel requested a review from hrkrshnn October 22, 2021 20:21
@shikharvashistha
Copy link
Contributor Author

You still have lots of syntax and style errors but overall the PR goes in a good direction so to help you out a bit I cleaned it and fixed the errors. Let's see if it passes tests now.

Please take a look at it and see if you'd like to add anything. Then squash commits so that each one is a single logical change (without separate commits for PR fixes).

Greetings of the day Mr @cameel,

Thanks for the help extended for all the valuable inputs that made me learn new things and at the same time making this pull possible.

Thanks & Regards
@shikharvashistha

hrkrshnn
hrkrshnn previously approved these changes Oct 25, 2021
View reviewed changes
Copy link
Member

@hrkrshnn hrkrshnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Two minor questions. I'll squash the commits and approve the PR, since changes are not needed to merge this.

liblangutil/Exceptions.cpp
if (_input == formatErrorSeverityLowercase(severity))
return severity;

return nullopt;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a throw? Fine either ways.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because we sometimes do use other severity values. This should really be cleaned up in #11959 but for now we have to accept other values here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. My issues are basically resolved. Feel free to press merge if you wish :)

@shikharvashistha @cameel @hrkrshnn
Added different colors for warning & error
cf00536 
Co-authored-by: shikharvashistha <shikharvashistha@yandex.com>
Co-authored-by: cameel <kamil.sliwak@codepoets.it>
@hrkrshnn
Copy link
Member

@shikharvashistha Thanks for the PR and for all the follow ups. I'll merge the PR now.

@hrkrshnn hrkrshnn merged commit 1da57d5 into ethereum:develop Oct 25, 2021
@cameel
Copy link
Member

cameel commented Oct 25, 2021

By the way, I checked how it works on a white terminal and looks like our output is completely broken so white used for INFO is the least of our problems. The messages themselves are invisible:

solc-errors-white-terminal

No one reported it so far so though.

@shikharvashistha
Copy link
Contributor Author

shikharvashistha commented Oct 25, 2021 •
edited
Loading

@shikharvashistha Thanks for the PR and for all the follow ups. I'll merge the PR now.

Greetings of the Mr. @hrkrshnn ,

Thanks for that label but I want to let you know that I didn't do it for hacktober fest 😂.

Thanks & Regards
@shikharvashistha

@shikharvashistha
Copy link
Contributor Author

shikharvashistha commented Oct 25, 2021 •
edited
Loading

By the way, I checked how it works on a white terminal and looks like our output is completely broken so white used for INFO is the least of our problems. The messages themselves are invisible:

solc-errors-white-terminal

No one reported it so far so though.

So should we change the color for info ? to something visible, orange for example 🤔 @cameel

@cameel
Copy link
Member

cameel commented Oct 25, 2021

Not really, it's not the info color that's the problem. We'd have to adjust colors to the terminal style somehow. It's harder to do and I'm not even sure if it's worth the effort (apparently not many people are using white terminals). I'd leave it as is unless someone complains.

@shikharvashistha
Copy link
Contributor Author

Not really, it's not the info color that's the problem. We'd have to adjust colors to the terminal style somehow. It's harder to do and I'm not even sure if it's worth the effort (apparently not many people are using white terminals). I'd leave it as is unless someone complains.

Ahh great.
Looking forward to contribute more to solidity 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cameel cameel cameel left review comments

@hrkrshnn hrkrshnn hrkrshnn approved these changes

Assignees
No one assigned
Labels
external contribution ⭐ hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Errors, warnings and infos should have different colors in terminal output
3 participants
@shikharvashistha @cameel @hrkrshnn