Document client.escapeIdentifier and client.escapeLiteral #2954
Conversation
Per brianc#1978 it seems that these client APIs are undocumented. Added documentation for these functions along with some examples and relevant links.
|
Fixed the typos, thank you for pointing them out!
Agreed. It should be fairly simple to do this, and it would be cleaner from a documentation perspective. I'll follow up with those changes |
These are standalone utility functions, they do not need a client instance to function. Changes made: - Refactored escapeIdentifer and escapeLiteral from client class to functions in utils - Update PG to export escapeIdentifier and escapeLiteral - Migrated tests for Client.escapeIdentifier and Client.escapeLiteral to tests for utils - Updated documentation, added a "utilities" page where these helpers are discussed **note** this is a breaking change. Users who used these functions (previously undocumented) on instances of Client, or via Client.prototype.
There was a problem hiding this comment.
I like the thinking here, but the best thing to do is leave them on both client and also export them on the root pg object and only document them on the pg. object. Removing stuff & causing backwards incompatible changes is a months long process w/ deprecation warnings and coordinating other breaking changes and weighing their impact. For the sake of the millions of users the best thing to do is keep it on both Client.prototype and add it to pg and then in another PR we could deprecate the old usage.
|
awesome! thank you - yeah we can just have the functions available in both places (with tests for both access paths) & we're good to goooo! I appreciate the work here - over the years I just got very, very cautious w/ breaking changes. :) |
These are standalone utility functions, they should not depend on a client instance. Changes made: - Refactored escapeIdentifer and escapeLiteral from client class to functions in utils - Re-exported functions on client for backwards compatibility - Update PG to export escapeIdentifier and escapeLiteral - Updated tests to validate the newly exported functions from both entry points - Updated documentation, added a "utilities" page where these helpers are discussed
|
@brianc no worries! a future PR or issue can be created for the breaking changes Updated with requested changes -- these updates are now backwards compatible. This is a matter of preference, but I chose to keep the functions in utils but re-export on client with a disclaimer that it is for backwards compatibility. Restored original tests for client escapes, added an additional test step for the other entry point in utils. Not sure how I feel about testing the second entry point in the client test suite, but the alternative is duplicating the tests for the utils test suite. |
| this.escapeIdentifier = escapeIdentifier | ||
| this.escapeLiteral = escapeLiteral |
There was a problem hiding this comment.
This is just new though, right? Why?
There was a problem hiding this comment.
This is new yes, as for why your previous comment suggested that the escape* methods be exported on pg, this adds them as exports to the pg object. On my end it looks like the github diff viewer annoyingly cuts off the part where it shows these belonging to PG, but looking at the full file it shows the rest of the context.
Is there an issue with exporting them this way?
Updated changes such that escapeIdentifier and escapeLiteral are usable via the client prototype Updated tests to check for both entry points in client
|
Hey sorry for the delay - was on holiday in Europe. Back now!! The code looks great & thanks for the docs updates as well. :) I'm running the tests now & if they pass I'll merge it in. ❤️ |
* Document client.escapeIdentifier and client.escapeLiteral Per brianc#1978 it seems that these client APIs are undocumented. Added documentation for these functions along with some examples and relevant links. * Fix typos in new docs * Migrate escapeIdentifier and escapeLiteral from Client to PG These are standalone utility functions, they do not need a client instance to function. Changes made: - Refactored escapeIdentifer and escapeLiteral from client class to functions in utils - Update PG to export escapeIdentifier and escapeLiteral - Migrated tests for Client.escapeIdentifier and Client.escapeLiteral to tests for utils - Updated documentation, added a "utilities" page where these helpers are discussed **note** this is a breaking change. Users who used these functions (previously undocumented) on instances of Client, or via Client.prototype. * Export escapeIdentifier and escapeLiteral from PG These are standalone utility functions, they should not depend on a client instance. Changes made: - Refactored escapeIdentifer and escapeLiteral from client class to functions in utils - Re-exported functions on client for backwards compatibility - Update PG to export escapeIdentifier and escapeLiteral - Updated tests to validate the newly exported functions from both entry points - Updated documentation, added a "utilities" page where these helpers are discussed * Ensure escape functions work via Client.prototype Updated changes such that escapeIdentifier and escapeLiteral are usable via the client prototype Updated tests to check for both entry points in client
Per #1978 it seems that these client APIs are undocumented. I added documentation for these methods along with some examples and relevant links.
I wasn't too sure what kind of structure to take with the documentation, such as documenting the usage via a client instance and the Client prototype. I can shuffle things around if there is a better way of expressing the usage.