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

Change SyntaxError to underline complete last token #2574

Merged

Conversation

fingolfin
Copy link
Member

This is an experimental patch, which changes the highlighting to syntax errors: instead of just pointing at the position we were when the syntax error occurred (which usually is at the end or just after the end of the problematic token), we now "underline" the whole token.

Example: Previously one got

gap> 1.x1;
Syntax error: Badly formed number in stream:1
1.x1;
  ^

With this PR

gap> 1.x1;
Syntax error: Badly formed number in stream:1
1.x1;
^^^

which I find slightly more helpful (esp. for longer expressions). For more examples, look at the changes to the test files.

Thoughts? Do people like or hate this? Or are neutral?

Note that SymbolStartPos and SymbolStartLine will be useful to implement something else we discussed in St Andrews: using the scanner+reader to tokenize and parse GAP code, w/o executing or coding it; instead, to only (programmatically) determine if it syntactically valid, and to extract the tokenization (e.g. for syntax highlighting in command line prompts). For that, we want to record start and end of tokens (we might want to do this in terms of an absolute seek position for a stream, though, but that's a separate concern), amongst other things.

@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements request for comments topic: kernel labels Jun 26, 2018
Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

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

I don't have difficulties with the current way of underlining either, but perhaps because I know already what to anticipate.

A useful way to see the suggested behaviour is to look at modified test files. For example, here

 function() quit; end;
-              ^
+           ^^^^

this PR underlines the whole keyword and makes the problem more visible, while here

 if false then ?what fi;
-                      ^
+              ^^^^^^^^^

the current behaviour points to a different location. Other examples can be seen in diffs. The suggested behaviour seems to be more user-friendly to me, and I like it.

Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

I am neutral in this respect,
the old and the new behaviour are equally good.

@olexandr-konovalov
Copy link
Member

@fingolfin what are those Travis CI failures however?

@fingolfin fingolfin force-pushed the mh/underline-syntax-error branch from b951593 to 617c19c Compare June 28, 2018 08:06
@fingolfin
Copy link
Member Author

@alex-konovalov should be fixed now. We'll see for sure after the tests cycled.

@codecov
Copy link

codecov bot commented Jun 28, 2018

Codecov Report

Merging #2574 into master will increase coverage by <.01%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #2574      +/-   ##
==========================================
+ Coverage    74.6%   74.61%   +<.01%     
==========================================
  Files         481      481              
  Lines      242383   242393      +10     
==========================================
+ Hits       180837   180853      +16     
+ Misses      61546    61540       -6
Impacted Files Coverage Δ
src/gapstate.h 100% <ø> (ø) ⬆️
src/scanner.c 93.2% <93.33%> (+0.16%) ⬆️
src/hpc/threadapi.c 43.48% <0%> (-0.11%) ⬇️
src/io.c 60.84% <0%> (+0.15%) ⬆️
src/objset.c 84.82% <0%> (+0.22%) ⬆️
hpcgap/lib/hpc/stdtasks.g 64.96% <0%> (+0.25%) ⬆️
src/iostream.c 63.49% <0%> (+1.14%) ⬆️

@frankluebeck
Copy link
Member

I agree that SymbolStartPos and SymbolStartLine will be useful.

Concerning the ^ marks my mild preference is for the old behaviour. In most cases the token is short and easy to detect from the trailing ^ mark, and for long tokens I find the huge number of ^ more confusing than helpful.

It would be nice if the code of the token could be underlined or colored instead of having the ^ marks.

@fingolfin
Copy link
Member Author

Yeah, coloring tokens is on my wish list, too (along with coloring tokens on the REPL, and when printing functions inside GAP)

@fingolfin fingolfin force-pushed the mh/underline-syntax-error branch from 617c19c to 259b921 Compare June 28, 2018 10:23
@ChrisJefferson
Copy link
Contributor

I quite like this, and also like the code related to tracking tokens. The ? output is very weird, but that' just because how we handle and parse ? is very weird :)

@ChrisJefferson ChrisJefferson merged commit c44b660 into gap-system:master Jul 4, 2018
@ChrisJefferson
Copy link
Contributor

I decided to merge this as I think there was a mild preference for merging, and also the debugging output gives us a useful test for checking if we continue tracking tokens correctly.

@fingolfin fingolfin deleted the mh/underline-syntax-error branch July 4, 2018 11:06
@fingolfin fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Sep 28, 2018
@fingolfin fingolfin added kind: discussion discussions, questions, requests for comments, and so on and removed kind: request for comments labels Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: discussion discussions, questions, requests for comments, and so on kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants