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

Error() should word-wrap error messages, not break them in the middle of a word #1197

Open
fingolfin opened this issue Mar 26, 2017 · 7 comments
Labels
kind: bug Issues describing general bugs, and PRs fixing them kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: error handling

Comments

@fingolfin
Copy link
Member

From a comment by @james-d-mitchell on issue #1195:

Error, IsomorphismTransformationMonoid: usage, the argument must be a semigroup w
ith a multiplicative neutral element, at /Users/jdm/gap/lib/semitran.gi:360 call
ed from
<function "unknown">( <arguments> )
 called from read-eval loop at *stdin*:2
type 'quit;' to quit to outer loop
brk>

Note how the word "with" is split after the "w". Here is an experiment I just did myself:

gap> Error("This is a very long text. Bla bla. More word. and it is present in one of the GAP root directories. and it is present in one of the GAP root directories");
Error, This is a very long text. Bla bla. More word. and it is present in one of the GAP root directori\
es. and it is present in one of the GAP root directories called from
not in any function at *stdin*:1
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk>

Note how this is not only breaking the message in the middle of a word (though at least with a \ indicating this), but it also breaks too late, and thus we end up with a very short text line between two longer ones (my guess is that GAP fails to take the string Error, into account for the wrapping).

Both of these issues should be fixed. Here is a suggestion:

  1. First try to break at a whitespace;
  2. if that fails, fallback to breaking after non-letter/digit symbols;
  3. if that fails, fallback to breaking anywhere.

The wrapping indicator \ probably should only be shown in the last case.

@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements labels Mar 26, 2017
@james-d-mitchell
Copy link
Contributor

I think it would also be an improvement to have the at /Users/jdm/gap/lib/semitran.gi:360 on a newline. The path to a file containing an error is often rather long, and having it split over more than one line makes it harder to read than necessary.

@markuspf
Copy link
Member

Since I have been hacking on the input/output code, I just want to make 2 points:

  • For things like the Jupyter kernel (and other IDE features), it would be quite good to keep as much information as possible around in a structured way (i.e. not mashed together as a string) when Error is called. For instance we could produce clickable links to help or code in Jupyter
  • If we mash together a string we should not put line breaks in but line break hints, and have the appropriate output code deal with things.

@james-d-mitchell
Copy link
Contributor

@markuspf I'm happy with any solution that makes the errors easier to read.

@fingolfin
Copy link
Member Author

@markuspf the Error function (or rather ErrorInner) already "knows" all these things in a structured way. I imagine for Jupyter, the best thing would be to replace ErrorInner with a custom function. Incidentally, the scscp and Browse package already do something like this for other reasons; so it might be a good idea to design a proper way to "hook up" a replacement for this function.

@olexandr-konovalov
Copy link
Member

In particular, SCSCP is doing this in order to catch an error message on the server and send it back to the client instead of triggering a break loop on the server.

@markuspf
Copy link
Member

markuspf commented May 4, 2017

@fingolfin thanks for the hint, I wasn't aware of that.

@fingolfin
Copy link
Member Author

As to the line break in the wrong place: My guess was right. ErrorInner does this (among other things):

    PrintTo("*errout*","Error, ");
    for x in earlyMessage do
        PrintTo("*errout*",x);
    od;

Now, each call to PrintTo opens stderr by calling OpenOutput, which pushes a new element onto the Output stack, prints, and pops the Output stack. Unfortunately, no state is retained across calls, and so the stream position is reset across calls, via this snippet from OpenOutput:

    STATE(Output)->pos      = 0;
    STATE(Output)->indent   = 0;

There are multiple ways to address this, each with its own pros and cons:

  1. This immediate problem could be addressed by fussing the multiple calls to PrintTo into a single one, like this: PrintTo("*errout*",Concatenation(["Error, "],earlyMessage));. Of course the same problem occurs again several times in ErrorInner. And calling Concatenation should better not trigger another error...
  2. We could add a hack to OpenOutput / OpenAppend to make the "position" for stderr persistent... of course strictly speaking, the "position" also changes when things are printed to stdout... so we'd probably have to keep both of these synced and persistent... yuch...
  3. As a variation of 2, we could try to query the terminal for the current position inside OpenOutput
  4. We could use an output stream for stderr to partially avoid the problem...
  5. Perhaps better to address the problem at its root, though, and simply disable this word wrapping logic?! For me, it has always been more of a nuisance than helpful... But of course that would also affect all our test files, it would be a BIG change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: error handling
Projects
None yet
Development

No branches or pull requests

4 participants