PR for Issue #9

[cphillips]

Testing are passing, however tests were changed

I also think

if (typeof (value) == 'number') {
return Number(value);

is a better choice to sanitize the value

[rpedela]

Thank for the PR! I love the use of Number(value). Pertaining to your question in the issue, I think we can bump the version to 1.1 and add docs to the README and release notes about the breaking change. In most cases, it should not break existing code and I think it is a excellent change for this library.

What do you think of making the following PR changes?

1. Since both PG and JS support NaN, Infinity, and -Infinity, could we support that? Based on my reading of the PG docs, those values should be treated as strings. On the JS side, I believe Number.isFinite(value) can distinguish between a real number and those special constants. Adding tests for those special constants would be great as well.

2. Do you think we should support BigInt too?

if (typeof value === 'bigint') {
    return BigInt(value)
}

3. Please use === and !== for comparisons as appropriate.

[cphillips]

Great feedback, updates made.
I apologize for the whitespace changes ( I'm using VS Code defaults ), I can remove if needed.

[rpedela]

Thanks for the changes. I have a couple more requests if you don't mind, and then I will be happy to merge. Great job!

1. quoteLiteral() returns a string. Can you change to Number(value).toString() and same for bigint?
2. Can you add tests for bigint, NaN, Infinity, and -Infinity for identifiers too?

[cphillips]

Thanks for the changes. I have a couple more requests if you don't mind, and then I will be happy to merge. Great job!

Happy to make any changes and thanks.

1. quoteLiteral() returns a string. Can you change to Number(value).toString() and same for bigint?

Unless I misunderstood the issue, the idea was to not escape literal numbers and bigint. I will agree that doing it in quoteLiteral is an oxymoron, but I'll need to place the logic somewhere. Maybe in the replace loop here? -> https://github.com/cphillips/node-pg-format/blob/master/lib/index.js#L217

2. Can you add tests for bigint, NaN, Infinity, and -Infinity for identifiers too?

I added a test here -> https://github.com/cphillips/node-pg-format/blob/master/test/index.js#L9, but maybe I'm not understanding where I should add the test.

[rpedela]

The reason to return a string is that quoteLiteral() powers format() and it is available in the public interface too. If a user has a complicated use case, they may want to call quoteLiteral() directly. Since this library is about constructing dynamic SQL strings correctly, the output for quoteLiteral() should always be a string. Whether the output string is escaped, surrounded by single quotes, depends on the input type. You are correct, we want to skip escaping numbers to avoid explicit casting in SQL. It is similar to identifiers. If a string identifier is lowercase, alphanumeric, and only underscores then we can skip escaping it, otherwise it must be surrounded by double quotes. I think the code is great, just need to call toString() for number and bigint.

There is a testIdentArray variable which is used for testing identifiers. The only difference is AbC. You could copy testArray and then add that one value, or change the identifier tests to also use testArray with that added array item. Doesn't matter to me.

[cphillips]

Completed.
Sorry for the question on number 1, I realize now my question made no sense, in my mind the result was going to be a quoted string.

[mattcan]

Thanks for fixing this bug folks, would love to see this get published!

[selmanozturk]

Thanks @cphillips. We are still suffering from this issue @rpedela. Do you planning to merge it?