Project

General

Profile

Bug #6919

fix ErrorManager.displayError in TempTableBuilder

Added by Constantin Asofiei over 1 year ago. Updated about 1 year ago.

Status:
Review
Priority:
High
Assignee:
Boris Schegolev
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:

6919.diff Magnifier (20.4 KB) Boris Schegolev, 11/21/2022 05:55 PM

Related issues

Related to Base Language - Bug #5761: replace ErrorManager.displayError with recordOrThrowError and fix recordOrShowError to morph into a recordOrThrowError in certain cases. New

History

#2 Updated by Constantin Asofiei over 1 year ago

  • Priority changed from Normal to High

FWD uses ErrorManager.displayError instead of going through recordOrShowError, in i.e. TempTableBuilder.tempTablePrepareImpl. For example:

def temp-table tt1 field f1 as int.
def var ht as handle.
create temp-table ht.
ht:create-like(temp-table tt1:handle).
run proc0.

procedure proc0.

ht:temp-table-prepare("").
catch e as progress.lang.error :
  message e.
end.

end.

In this case, the CATCH block forces a OO structured exception to be raised, while FWD will just display it. OE does not raise an ERROR condition.

This task is meant to test and fix all cases of ErrorManager.displayError in TempTableBuilder and AbstractTempTable.

#3 Updated by Constantin Asofiei over 1 year ago

  • Related to Bug #5761: replace ErrorManager.displayError with recordOrThrowError and fix recordOrShowError to morph into a recordOrThrowError in certain cases. added

#4 Updated by Eric Faulhaber over 1 year ago

  • Assignee set to Boris Schegolev

#5 Updated by Boris Schegolev over 1 year ago

  • Status changed from New to WIP
  • % Done changed from 0 to 90
  • File 6919.diffMagnifier added

I am attaching my changes for review. This should be the complete implementation, I will do more testing tomorrow.

#6 Updated by Ovidiu Maxiniuc over 1 year ago

Review of 6919.diff.

Good job! There were a lot of methods to check.

Interesting findings. I was not expecting in several cases that only the message to be displayed without raising the error level. It has a logical part though: instead of checking the system error, the ABL programmer just need to check the logical value returned by the method.

I see you added static import for ErrorManager. There are very few places where this is done in FWD, the places where it is done is due the silent blocks. There is a chance this was done automatically by the IDE. I do not say it is a bad thing, but I think we should be consistent. Please either use static calls for all the static calls to ErrorManager or remove the import and use the class qualification for static methods where recordOrShowError() is invoked.

TempTableBuilder:3275: Please add back the secondary space between the phrases. I am also not a great fan of this, but this is the error message in legacy application so we are forced to keep it. Also, please try to keep the length of code lines smaller than 110 char limit set by GCD code standard (there is another occurrence at line 3376).

I do not see other major issue so after fixing at least the double space at TempTableBuilder:3275, the patch can be committed to 3821c (at least this is the branch the attached .diff fit for me).

LE: of course, do not forget to add the history-entry in the file header.

#7 Updated by Boris Schegolev over 1 year ago

  • Status changed from WIP to Review
  • % Done changed from 90 to 100

Ovidiu Maxiniuc wrote:

I see you added static import for ErrorManager. There are very few places where this is done in FWD, the places where it is done is due the silent blocks. There is a chance this was done automatically by the IDE. I do not say it is a bad thing, but I think we should be consistent. Please either use static calls for all the static calls to ErrorManager or remove the import and use the class qualification for static methods where recordOrShowError() is invoked.

I did that on purpose to keep the lines short. I reverted that, now method calls are qualified with ErrorManager. so that we are consistent with the rest of the codebase.

TempTableBuilder:3275: Please add back the secondary space between the phrases. I am also not a great fan of this, but this is the error message in legacy application so we are forced to keep it.

Honestly, I thought that was a typo :)

LE: of course, do not forget to add the history-entry in the file header.

Done. Thank you for review and hints!

Pushed revision 3821c/14398.

#8 Updated by Eric Faulhaber about 1 year ago

Is there anything left to do on this task, or can we close it?

#9 Updated by Boris Schegolev about 1 year ago

My understanding is that we can close it.

Also available in: Atom PDF