Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parenthesize negative numbers in ToField instances #145

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ChickenProp
Copy link

Closes #143.

This also switches the Scientific instance to use a ByteString builder instead of a Text.Lazy builder. Lazy text dates back to 87135c1, when scientific-0.2.0.2 only provided that option. Now we require scientific >= 0.3.7.0, which also provides the ByteString builder. The alternative was to reimplement parenNegatives for this instance, which also wouldn't be too bad.

For types with signed zeros, -0 isn't parenthesized. I wasn't sure how to detect it ("check whether the rendered value starts with -" should work but breaking out of Builder feels bad for performance?), and I'm not aware of any situation where -(0::type) would be different than (-0)::type. I could easily imagine I'm missing something though.

The use of a lazy text builder dates back to 87135c1, when
scientific-0.2.0.2 only provided that option. Now we require scientific
>= 0.3.7.0, which also provides the `ByteString` builder.
This is necessary because in PostgreSQL, `-` is a unary operator that
has lower precedence than the `::` operator, and that can cause problems
at the edge of allowed ranges.

For example, `-32768::int2` is parsed as `-(32768::int2)`, which throws
an "out of range" error, even though `(-32768)::int2` is accepted.

Closes haskellari#143.
Copy link
Collaborator

@phadej phadej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any tests.

@ChickenProp
Copy link
Author

Ah, I saw there were no tests of ToField, but didn't see that there were tests that involved connecting to a database. (I didn't have DATABASE_CONNSTRING in my environment and tests just passed, the message saying to set that var shows up in the log file but not the terminal output. And I guess I just didn't look closely at Main.hs.)

I've added a test now, let me know if you want something more thorough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ToField for negative numbers should be parenthesized
2 participants