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

repl: use better uncaught exceptions indicator #29676

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 23, 2019

This switches "Thrown:" with "Uncaught" to outline clearer that the
thrown error is not caught. It will also mark the error in red in
case the terminal supports colors / if colors are activated for the
repl.

Since this should only be relevant for the end user, should this be semver-major or is it fine as semver-minor?

Update: I removed the colors as requested below!

Before
image
image

After (with different color themes and on different terminals)
image
image
image
image
image

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Sep 23, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@BridgeAR
Copy link
Member Author

@nodejs/repl PTAL

@BridgeAR BridgeAR added the review wanted PRs that need reviews. label Sep 25, 2019
@jasnell
Copy link
Member

jasnell commented Sep 25, 2019

Generally fine with this approach but the change in output may make this semver-major :-/

@BridgeAR
Copy link
Member Author

@nodejs/tsc PTAL and provide your opinion about the semverness. It is not immediately clear what this should ideally be.

@BridgeAR
Copy link
Member Author

I added some before and after screenshots in the description above.

@jasnell
Copy link
Member

jasnell commented Sep 26, 2019

btw, fwiw I really dislike having to potentially treat these kinds of usability improvement changes to console output as semver-major but we have seen in the past that such changes can break existing code. I would be definitely amenable to any policy change or approach to handling these things that would allow us to avoid treating them as semver-major

@Trott
Copy link
Member

Trott commented Oct 1, 2019

Maybe a CITGM run will help make a decision for us? I wouldn't expect this in particular to break anything in CITGM, but if it does, that's certainly an argument for semver-major.

@devsnek
Copy link
Member

devsnek commented Oct 1, 2019

i like changing it to uncaught, not a huge fan of the red though. red kills readability on pretty much everything that isn't green thanks to the magic of our eyes and rgb.

@Trott
Copy link
Member

Trott commented Oct 4, 2019

Needs a rebase. Might make sense to split the wording change and the color change into separate PRs so any controversy about one won't delay the other.

@lholmquist
Copy link
Contributor

Agree that these are 2 separate concerns, and would be nice if they were broken into different PRs

@BridgeAR
Copy link
Member Author

I would prefer to keep the change together and not splitting this up.

I updated the colors to use bold bright red. This seems significantly better readability wise.

Before:
image

After:
image

I also added further screenshots at the top where I compare different color themes and terminals.

@devsnek @lholmquist PTAL

@BridgeAR
Copy link
Member Author

@nodejs/repl @nodejs/util PTAL

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@antsmartian
Copy link
Contributor

antsmartian commented Nov 21, 2019

I feel red color (with bold) is actually making it harder to read (on black terminals), IMHO.

@devsnek
Copy link
Member

devsnek commented Nov 21, 2019

Can the colors be a separate PR? I'd love for the uncaught stuff to get in since that seems pretty uncontroversial.

@BridgeAR
Copy link
Member Author

@devsnek @antsmartian @lholmquist does either of you have a suggestion what colors might be good to use instead? I will separate the colors from the rest of the PR even though I personally think they belong somewhat together.

Right now it's not immediately clear when an error is thrown and when not. Using a color indicator overcomes that problem. Thus, I expect most users to profit from the current color suggestion. We could also use a predefined background color, I personally do not like that idea all that much though.

This switches "Thrown:" with "Uncaught" to outline clearer that the
thrown error is not caught. It will also mark the error in red in
case the terminal supports colors / if colors are activated for the
repl.
@BridgeAR
Copy link
Member Author

I just pushed another commit that removes the colors.

@Trott
Copy link
Member

Trott commented Nov 26, 2019

If we have informed volunteers to do so, probably a good idea to get review by some accessibility folks to make sure default color contrast is sufficient, color choices aren't a problem for people with color vision deficiency, etc. They wouldn't even need to necessarily review the code to add value--they could evaluate the screenshots above. Evaluating the range of possibilities by reviewing the code or experimenting with the result of this PR would be even better, of course.

We don't have a specific accessibility group as far as I know, but I'm going to guess that there might be people with that kind of knowledge on the website teams, so... @nodejs/website @nodejs/website-redesign

@Trott
Copy link
Member

Trott commented Nov 26, 2019

Whoops, never mind if the colors have been removed? Sorry, everyone.

@antsmartian
Copy link
Contributor

@BridgeAR: I don't have any strong suggestion on the colors. But, can't we make it configurable with some config, so that the user can decide what color he/she wants to keep for the errors/stacktrace etc?

@BridgeAR BridgeAR requested a review from devsnek December 2, 2019 18:10
@BridgeAR BridgeAR changed the title repl: add colors for uncaught exceptions and use a better indicator repl: use better uncaught exceptions indicator Dec 2, 2019
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 2, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 3, 2019

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels Dec 6, 2019
BridgeAR added a commit that referenced this pull request Dec 6, 2019
This switches "Thrown:" with "Uncaught" to outline clearer that the
thrown error is not caught.

PR-URL: #29676
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 6, 2019

Landed in 6c40cb2 🎉

@BridgeAR BridgeAR closed this Dec 6, 2019
targos pushed a commit that referenced this pull request Dec 9, 2019
This switches "Thrown:" with "Uncaught" to outline clearer that the
thrown error is not caught.

PR-URL: #29676
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
This switches "Thrown:" with "Uncaught" to outline clearer that the
thrown error is not caught.

PR-URL: #29676
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
This switches "Thrown:" with "Uncaught" to outline clearer that the
thrown error is not caught.

PR-URL: #29676
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants