Skip to content

Type 'double precision' mapped to String#345

Merged
dhmlau merged 1 commit intoloopbackio:masterfrom
ataft:patch-2
Oct 19, 2018
Merged

Type 'double precision' mapped to String#345
dhmlau merged 1 commit intoloopbackio:masterfrom
ataft:patch-2

Conversation

@ataft
Copy link
Copy Markdown
Contributor

@ataft ataft commented Oct 18, 2018

On line 144, dataType "real" and "double precision" are changed to "float", but buildPropertyType has no case for "FLOAT":
https://github.com/strongloop/loopback-connector-postgresql/blob/master/lib/discovery.js#L144

This change simply adds a case statement for 'FLOAT'

@slnode
Copy link
Copy Markdown

slnode commented Oct 18, 2018

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@dhmlau
Copy link
Copy Markdown
Member

dhmlau commented Oct 18, 2018

@ataft, thanks for your contribution.
Could you please fix the commit message? Looks like line 3 of the commit message is longer than 72 characters.

**************************************************
**
**  Linting commit logs
**
**  1 problems found:
**    962fa56 - Type 'double precision' is mapped to String: line 3 longer than 72 characters.
**
**************************************************

Also, could you please add some test cases as well? Thanks.

@ataft ataft changed the title Type 'double precision' is mapped to String Type 'double precision' mapped to String Oct 18, 2018
@ataft
Copy link
Copy Markdown
Contributor Author

ataft commented Oct 18, 2018

@dhmlau I'm not so great with git and don't know how to change the commit message.

With regards to the tests, also not my strong suit, but the current "postgresql.discover.test.js" should cover this the 'double precision' type:
https://github.com/strongloop/loopback-connector-postgresql/blob/master/test/postgresql.discover.test.js#L113

@dhmlau
Copy link
Copy Markdown
Member

dhmlau commented Oct 19, 2018

@ataft, see if you can follow this instruction to change the commit message https://help.github.com/articles/changing-a-commit-message/. If not, don't worry, I can modify it for you.

Thanks for pointing out the existing test.

@dhmlau
Copy link
Copy Markdown
Member

dhmlau commented Oct 19, 2018

Let's get the commit message fixed so that CI can run successfully first. Thanks!

@ataft
Copy link
Copy Markdown
Contributor Author

ataft commented Oct 19, 2018

Please modify, I cannot figure it out since I just made the change in the browser (not using command line). Can you also modify my other PR #347? Thanks!

@dhmlau
Copy link
Copy Markdown
Member

dhmlau commented Oct 19, 2018

@slnode test please

Copy link
Copy Markdown
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

The change looks reasonable to me. But would like to get at least another review.

Copy link
Copy Markdown
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

LGTM

@dhmlau dhmlau merged commit c62fa58 into loopbackio:master Oct 19, 2018
@dhmlau
Copy link
Copy Markdown
Member

dhmlau commented Oct 19, 2018

Thanks @ataft. Your PR has landed. Thanks for the contribution!

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.

4 participants