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

DO NOT MERGE #7

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

DO NOT MERGE #7

wants to merge 17 commits into from

Conversation

shutej
Copy link

@shutej shutej commented Jan 7, 2015

Note the s/relops/shutej/g everywhere. This needs to get fixed before merging.

This adds REAL, DOUBLE PRECISION, and BOOLEAN support.

@0x6e6562
Copy link
Member

0x6e6562 commented Jan 7, 2015

Sure, I'll wait before it's in a mergeable state before beginning a review.

I'll be interested to see how smoothly the Boolean type works across the three back ends.

@shutej
Copy link
Author

shutej commented Jan 8, 2015

it ... doesn't work across all backends. BIT is another type that's
relevant than is probably ANSI, i think. but where BOOLEAN or BOOL exist,
this supports it.

basically the LAST step of the review is subbing the shutej for relops
again. i'll do that when you approve?

Jeremy

On Wed, Jan 7, 2015 at 6:53 PM, Ben Hood notifications@github.com wrote:

Sure, I'll wait before it's in a mergeable state before beginning a review.

I'll be interested to see how smoothly the Boolean type works across the
three back ends.


Reply to this email directly or view it on GitHub
#7 (comment).

@0x6e6562
Copy link
Member

0x6e6562 commented Jan 8, 2015

Fundamentally the patch is a good idea, but to get it into a mergable state, I think the following will be need to be addressed:

  • A test case (or an addition to an existing test case) that exercises the change against a real backend - take a look at the existing back end tests if you're unsure about this.
  • Having this work across all backends would be nice as well, but that might not be possible.
  • Your gofmt settings appear to have made some tab changes, in particular in schema.tmpl, which is used for a lot of the generated code - this has created quite a noisy patch.

@shutej
Copy link
Author

shutej commented Jan 8, 2015

Right, I'll install MySQL and test this sucker. :-)

I can un-bork schema.tmpl, but my "settings" are ... the defaults. I
didn't know there WERE settings.

Jeremy

On Thu, Jan 8, 2015 at 9:59 AM, Ben Hood notifications@github.com wrote:

Fundamentally the patch is a good idea, but to get it into a mergable
state, I think the following will be need to be addressed:

  • A test case (or an addition to an existing test case) that exercises
    the change against a real backend - take a look at the existing back end
    tests if you're unsure about this.
  • Having this work across all backends would be nice as well, but that
    might not be possible.
  • Your gofmt settings appear to have made some tab changes, in
    particular in schema.tmpl, which is used for a lot of the generated
    code - this has created quite a noisy patch.


Reply to this email directly or view it on GitHub
#7 (comment).

@0x6e6562
Copy link
Member

0x6e6562 commented Jan 8, 2015

I meant "settings" figuratively - I was just pointing out that there is a divergence in the formatting, not that I know for sure where it is coming from.

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.

2 participants