SWI-Prolog / packages-jpl

JPL: The Prolog <-> Java interface
BSD 2-Clause "Simplified" License
54 stars 33 forks source link

CLEANUP: Exception generation moved out of jpl.pl to a separate file & module: jpl_exceptions.pl #57

Closed dtonhofer closed 4 years ago

dtonhofer commented 4 years ago

A new module, "jpl_exceptions" (file "jpl_exceptions.pl"), has been added and is used by module "jpl".

This module exports predicates to throw exceptions in a standard manner to simplify the code of the throwing predicate and avoid repetition of error-prone term constructions sprinkled throughout the source of jpl.pl.

Moreover, cleartext message to be carried by the exception term is no longer found embedded in the throwing predicate, but is specified there by a an "error code" atom, like 'ea01'.

Nevertheless, a cleartext message can still be passed in place of an "error code" atom by the throwing predicate: Any code that doesn't resolve to a cleartext message is inserted "as is" into the exception error term that will be thrown.

JanWielemaker commented 4 years ago

Thanks. I do have some problems with this though.

Overall, I think it would be a good move to use library(error) for the JPL errors. We are faced with the fact that, as is, library(error) does not support context/comment and jpl heavily uses this. That gives two options: drop this from JPL or extend library(error) with errors that include more context. I've been struggling with the latter for some time, but I'm not really sure how. Possibly we should discuss so on Discourse?

dtonhofer commented 4 years ago

Thanks Jan.

The "error codes" are just used to retrieve the cleartext error messages that were formerly in-line - they have no other function. The predicates in jpl_exceptions.pl collect the code that builds the exception/error terms, which was repeated again and again all over jpl.pl (I think I found a couple of mis-constructions, too). Evidently an application of the "don't repeat yourself" principle. They throw explicitly but they could also use library(error) - if library(error) provided a way to set the context information of the error term.

So instead of this code in jpl_exceptions.pl, which mirrors what is done in jpl.pl:

throw_existence_error(Pred,Type,Term,ExCode) :-
   (exception_code(ExCode,ExText) -> true ; (ExText = ExCode)),
   throw(error(existence_error(Type,Term),context(Pred,ExText))).

one could use existence_error/2 from library(error), but it doesn't allow to set the context ...

throw_existence_error(Pred,Type,Term,ExCode) :-
   (exception_code(ExCode,ExText) -> true ; (ExText = ExCode)),
   existence_error(Type,Term).

(but is there a difference to a debugger when one calls throw/1 directly and when one calls throw/1 through a helper predicate one ore two frames down the stack?)

For the modular behaviour - wouldn't Prolog just pull in jpl_exceptions.pl when it loads jpl.pl? I was looking at this as "cutting a large bunch of code up into task-specific element" - that's what one would do with a huge class - separate out task-specific elements into another class (or an inner class), possibly as a set static functions.

I'm for an extension of library(error) which allows to specify the context term - it would absolutely make sense.

But yes, let's discuss on Discourse.

JanWielemaker commented 4 years ago

Misread how the error codes were used. Sorry. As for the module, this is a module that supports only one specific other module (jpl). That adds little. It does pollute the library(X) name space though as well as the name space for autoloadable predicates. If we want to chop up jpl.pl, the way to go would be to create a subdirectory jpl in the library, put the helper module(s) there and load them from library(jpl). I'm not against that, but it also seems a bit overkill unless the people involved in JPL go for a serious cleanup/rewrite. I leave that to @ssardina and @anionic.

dtonhofer commented 4 years ago

Ok. So, fuse code ... or a subdirectory. Let's see what the @ssardina and @anionic say.

But the idea of extending library(error) still stands. Ah... should I write an initial post on Discourse?

Also, the next step I wanted to perform for jpl.pl was to clean up & fix the classname parsing (it seems to have grown organically) so that I can actually connect to LibreOffice. Next week, hopefully.

JanWielemaker commented 4 years ago

But the idea of extending library(error) still stands. Ah... should I write an initial post on Discourse?

Yes, please do

connect to LibreOffice

Great!

anionic commented 4 years ago

jpl.pl was originally it at least seven parts, and jpl.c in two, but early on they were combined and renamed to simplify the source distribution and building, and although I am barely active in maintaining them, I believe they should stay like that (I use PDT and Eclipse to maintain some .pl files > 10k lines and a .java > 50k lines and neither know nor care whereabouts in each file a predicate or method is, relying on def/use navigation and searching. I'd rather have everything in one file with smarter navigation). N.B. I despair of Git's stupid diff algorithm, which cannot distinguish semantically significant changes from housekeeping changes resulting from constant best-practice code refactoring, otherwise I might have contributed to ongoing cleanup/rewrite. Many years ago I wrote and used a multi-level diff of Prolog sourcetext which progressively abstracted away from changes to indentation, whitespace, comments, variable names, syntactic sugar, DCG notation, order of predicates in file etc. Could something like this be plugged into Git? As it is, Git inhibits us from even tidying the formatting. @dtonhofer knowing what you now know about JPL's use of the JNI notation for static nested classes (e.g. java.awt.geom.Path2D$Float), are you unable to use the LibreOffice Java API from JPL?

dtonhofer commented 4 years ago

@anionic "unable to use the LibreOffice Java API from JPL?" Yes - at least with the JPL with SWI-Prolog 8.1.32-10, the problem is the $ at the start of a non-nested classname from LibreOffice, which JPL doesn't accept. Further field, JPL also doesn't accept "Java Identifer Characters" in the Classnames, just Alphanumerics. That's why I wanted to go through the DCG and also add some unit tests. This is good for me get some DCGs practice, too.

So~~~~ ... should I fuse the jpl_exception.pl into jpl.pl?

JanWielemaker commented 4 years ago

So~~~~ ... should I fuse the jpl_exception.pl into jpl.pl?

A good first step is probably to fuse them. Ideally, replace the ExCode identifiers with readable Prolog terms. Numbers are not commonly used in symbolic languages.

That will refactor the code good enough to do further integration with an extended library(error) in one place or turn that part of the exercise into a couple of simple replace commands.

dtonhofer commented 4 years ago

Ok for fuse, I will submit a new request once done.

But I don't see how to replace the ExCodes ... they are just meaningless atoms identifying the cleartext messages - I just went with ea, eb, ec, ... according to the predicate and with contiguous natural for the messages inside every predicate. Do you mean replace them with atoms that are some approximation of the message text?

dtonhofer commented 4 years ago

A new pull request has been created with everything in jpl.pl