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

Improve evalstr to show syntax warnings #1059

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

fingolfin
Copy link
Member

Resolves #1058

Copy link

codecov bot commented Oct 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.97%. Comparing base (5cbad84) to head (379eb60).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1059      +/-   ##
==========================================
- Coverage   75.04%   74.97%   -0.07%     
==========================================
  Files          55       55              
  Lines        4556     4560       +4     
==========================================
  Hits         3419     3419              
- Misses       1137     1141       +4     
Files with missing lines Coverage Δ
src/GAP.jl 86.08% <100.00%> (+0.50%) ⬆️
src/ccalls.jl 99.20% <100.00%> (+<0.01%) ⬆️

... and 2 files with indirect coverage changes

Copy link
Member

@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.

This is a good idea: Turn a real GAP error into a Julia error, and handle the GAP warning by just printing it.

As far as I understand, we have the following.
The syntax errors happen in the evalstr_ex call, and the buffer gets cleaned on the outer level, in evalstr.
Thus we will still keep the warning in the buffer if we call evalstr_ex directly. (Note that eval_str_ex is documented, and is mentioned in the docstring of evalstr.)

Logically, it would be better if evalstr_ex (or some helper function) would collect all GAP errors and warnings, clean the error buffer, and return also the collected error an warning information. Then the status of GAP would be clean after the evalstr_ex call. And evalstr would still be able to throw the Julia exception and to print the warning.

Is this reasonable?

@fingolfin
Copy link
Member Author

I like the idea of adding the last error as a second argument to evalstr_ex. However it would be breaking change. @ThomasBreuer discussed this and agreed to merge this as-is for now, and we'll change evalstr_ex in a later PR for the next release.

@fingolfin fingolfin merged commit a8f197a into oscar-system:master Oct 28, 2024
19 of 21 checks passed
@fingolfin fingolfin deleted the mh/last_error branch October 28, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

irritating GAP warning/error handling
2 participants