SWI-Prolog / packages-pengines

Pengines: Prolog engines
11 stars 13 forks source link

Fixes in pengines:output_result for CORS and write_result hook #46

Closed ridgeworks closed 5 years ago

ridgeworks commented 5 years ago
  1. Currently CORS support is only implemented for the json format. It really should be applied to all output when the http:cors setting is enabled (including any error responses generated by the http server libraries). The same philosophy should apply to disable_client_cache/0.

  2. The write_result/3 hook is not called when the format type is prolog due to clause ordering of output_result/3. The clause that calls the hook should be first (analogous to the event_to_json hook in event_term_to_json_data/3). A benefit is that attributed variables can by suitably formatted in response data via the hook.

JanWielemaker commented 5 years ago

Thanks. Merged after small edit: move cors and cache control out of the catch as this is not needed and I try to avoid complex queries as arguments to meta-predicates (they make debugging harder and are slower). Added comment to begin with FIXED:. Any commit starting with [A-Z]+: goes into the changelog.

Note that GitHub fails to pick up the modified commit. It is merged anyway.

ridgeworks commented 5 years ago

Thanks.

I did wonder about the placement, i.e., inside or outside the catch. So it sounds like the recommended practice to keep the 'catch' Goal to a single term and create an additional predicate if necessary.

On Feb 25, 2019, at 3:13 PM, Jan Wielemaker notifications@github.com wrote:

Thanks. Merged after small edit: move cors and cache control out of the catch as this is not needed and I try to avoid complex queries as arguments to meta-predicates (they make debugging harder and are slower). Added comment to begin with FIXED:. Any commit starting with [A-Z]+: goes into the changelog.

Note that GitHub fails to pick up the modified commit. It is merged anyway.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SWI-Prolog/packages-pengines/pull/46#issuecomment-467165259, or mute the thread https://github.com/notifications/unsubscribe-auth/AG5Uwe34retBUncr9YDxjJLM1MuVjdQ1ks5vREPmgaJpZM4bQqb5.

JanWielemaker commented 5 years ago

So it sounds like the recommended practice to keep the 'catch' Goal to a single term and create an additional predicate if necessary.

That is my personal style preference. That is, some is personal (I dislike complex nesting), part is a limitation by the current implementation that makes debugging such control structures relatively hard and their execution a little slower (irrelevant, except for tight loops).

JanWielemaker commented 5 years ago

This pull request has been mentioned on SWI-Prolog. There might be relevant details there:

https://swi-prolog.discourse.group/t/attributed-variables-and-pengines/259/2