Skip to content

Show colored error messages for the run.dlang.io backend #1981

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wilzbach
Copy link
Member

Pretty horrible JS magic as most of the previous output from DPaste isn't supported, but it's not a huge deal as the user is either just interested in the normal output or the error message.

In a follow-up, I might also add support for colored error messages like run.dlang.io has

@dlang-bot
Copy link
Contributor

dlang-bot commented Dec 14, 2017

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close Bugzilla Description
18306 No compliation errors shown when running modified examples

@wilzbach wilzbach requested a review from andralex as a code owner December 14, 2017 07:05
@wilzbach
Copy link
Member Author

In a follow-up, I might also add support for colored error messages like run.dlang.io has

Turns out it's not that hard with AnsiUp (that's the lib we used for run.dlang.io):

image

Frontpage

image

More colors:

image

The colors are CSS classes and can be tweaked.

@wilzbach wilzbach changed the title Show error messages for the run.dlang.io backend Show colored error messages for the run.dlang.io backend Dec 14, 2017
@JinShil
Copy link
Contributor

JinShil commented Dec 14, 2017

That cyan looks pretty bad on white. Maybe a blue instead. Doesn't really matter to me, as long as it has more contrast.

@CyberShadow
Copy link
Member

Do we really need to add another HTTP request for that CSS file?

@wilzbach wilzbach force-pushed the show-error-messages branch from 8c8b9ca to edfd887 Compare December 15, 2017 09:03
@wilzbach
Copy link
Member Author

Do we really need to add another HTTP request for that CSS file?

No. Included into style.css.
Should I also inline the regarding JS code?

@wilzbach wilzbach force-pushed the show-error-messages branch from 62a9763 to 463c8b0 Compare December 15, 2017 09:11
@CyberShadow
Copy link
Member

Thanks. Keeping things in separate files would be more amenable if they were concatenated during the build, but even so, I'm not sure it's worth it.

@CyberShadow
Copy link
Member

Should I also inline the regarding JS code?

Honestly, I think doing the ANSI->HTML translation ought to be done by the server. There is no value in sending raw ANSI over the wire, as far as I can see.

@wilzbach
Copy link
Member Author

There is no value in sending raw ANSI over the wire, as far as I can see.

  • Security: sending HTML means that it easily holes for injecting arbitrary HTML
  • Simplicity: I didn't know of any D ANSI 2 HTML implementation. There are plenty in JS (main point)

But I do agree that adding the ansi JS increases the size of the sources needed for dlang.org :/

@wilzbach wilzbach force-pushed the show-error-messages branch from 463c8b0 to 5f8f406 Compare December 18, 2017 05:59
@wilzbach wilzbach closed this Dec 18, 2017
@wilzbach wilzbach deleted the show-error-messages branch December 18, 2017 06:01
@wilzbach wilzbach restored the show-error-messages branch February 3, 2018 20:24
@wilzbach wilzbach reopened this Feb 3, 2018
@wilzbach wilzbach force-pushed the show-error-messages branch 2 times, most recently from bbd989c to 4d28b0d Compare February 3, 2018 21:07
@wilzbach wilzbach force-pushed the show-error-messages branch from 4d28b0d to 07f0c2b Compare February 3, 2018 21:12
@wilzbach
Copy link
Member Author

wilzbach commented Feb 3, 2018

On a second thought, I'm not planning on adding this HTML output to the backend for the reasons from above (security:holes for injecting arbitrary HTML + simplicity: I didn't know of any D ANSI 2 HTML implementation. There are plenty in JS).
I rebased this to include a uglified version of AnsiUp. It's 10K without compression and and 2.3K with compression. Imho that's more than worth it for nice, colorized error messages without investing a lot of time to reinvent the wheel.

@CyberShadow
Copy link
Member

I'm not going to block this PR, but I still don't like this.

DMD uses a very tiny subset of ANSI, parsing which should be absolutely trivial by hand. This both puts under the question of including an entire library someone else has written, and the argument that doing it on the server is too hard.

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

Successfully merging this pull request may close these issues.

4 participants