Simplify setting up Header objects #85
Conversation
Signed-off-by: Matthew Peveler <matt@popsql.com>
| 'Prepared-Statement', | ||
| ]; | ||
| const legacyPrefix = { | ||
| 'PREPARE': 'Prepared-Statement', |
There was a problem hiding this comment.
This was the only header to not follow the above simple rule, and I left it here to keep the objects overall backwards compatible, though the client itself uses the new PREPARED_STATEMENT key instead. I'm not sure anyone is relying on the header objects directly, so could potentially just delete this.
lib/presto-client/headers.js
Outdated
| // with the key being the value uppercased and dashes replaced | ||
| // with underscores, e.g. `Max-Size` becomes | ||
| // `{ MAX_SIZE: 'X-Presto-Max-Size' }` on Headers object. | ||
| const commonPrefix = [ |
There was a problem hiding this comment.
Is this commonSuffix? These parts become the suffix part of header fields (with Presto/Trino specific prefixes).
There was a problem hiding this comment.
Or commonFields makes sense to me.
There was a problem hiding this comment.
I went with commonPrefix to indicate that these were fields that would be prefixed by the common X-${engine} string. However, I'm fine calling it commonSuffix instead. commonFields would also work, though I like using Suffix (or Prefix) as that gets mirrored into the legacySuffix variable and how that works.
Done in 1d0cc0c
|
👍 |
PR modifies the
headers.jsfile so that the header objects are generated from shared array of values, so that adding a new header is only done once, vs twice, to avoid duplication and potential mispellings and such. I also rewrote the overall object to be a regular object instead of a function with additional properties, as the latter struck me as a bit odd / unnecessary vs just using a regular object.This is a prep PR for adding additional header values (e.g.
language,role) that the adapter currently does not support.