[gh-152] Revert change that was made to postgres schema parsing that cau... #194

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

apinstein commented Nov 17, 2011

...ses regressions and has no tests.

The original "fix" that I reverted was undocumented, unreproducible, etc. I have no idea how to even verify what he expected from the original ticket. Clearly it was some edge case involving quotes in default values.

I have reverted that change out to restore normal behavior until the original poster can provide some kind of test data or a better patch.

Member

fzaninotto commented Nov 18, 2011

Again, can you reference the original issue/commit?

Contributor

apinstein commented Nov 18, 2011

The issue has links/info to the trac system which has all of the relevant info.

Sent from iPhone.

On Nov 18, 2011, at 6:46 AM, Francois Zaninottoreply@reply.github.com wrote:

Again, can you reference the original issue/commit?


Reply to this email directly or view it on GitHub:
#194 (comment)

Owner

willdurand commented Feb 28, 2012

@apinstein can you write some unit tests? You should fix indentation too

Contributor

apinstein commented Mar 2, 2012

Yeah I will try. IIRC I gut stuck previously trying to get the tests to run...

On Feb 28, 2012, at 3:37 AM, William DURAND wrote:

@apinstein can you write some unit tests? You should fix indentation too


Reply to this email directly or view it on GitHub:
#194 (comment)

[gh-152] Revert change that was made to postgres schema parsing that
causes regressions and has no tests. Add tests for my case. I do not
understand the case that the person who introduced the bug was trying to
solve.
Contributor

apinstein commented Mar 3, 2012

Ok, I got tests working for this!

I suppose you should get the author of e2067a8#L1R225 (niklas) to now add a test for his case and fix it in a way so all of our tests pass :)

Contributor

apinstein commented Mar 3, 2012

Also you need the "reverse_bookstore" table to exist in the DB for this test to run ok... I wasn't sure where to add that info since there is really no info in the README about testing pgsql.

But if as postgres you run:

create database reverse_bookstore;

That's all that is needed to run my tests.

Owner

willdurand commented Mar 3, 2012

Well done. I'll review your work soon.

To run the propel's test suite on pgsql isn't easy unfortunately but it should be better with Propel2.

Le 3 mars 2012 à 17:21, Alan Pinsteinreply@reply.github.com a écrit :

Also you need the "reverse_bookstore" table to exist in the DB for this test to run ok... I wasn't sure where to add that info since there is really no info in the README about testing pgsql.

But if as postgres you run:

create database reverse_bookstore;

That's all that is needed to run my tests.


Reply to this email directly or view it on GitHub:
#194 (comment)

Contributor

apinstein commented Mar 3, 2012

Yea I just ran my single test, that helped! Fortunately there was a similar MySQL test to clone or I would've been really stuck.

Alan

On Mar 3, 2012, at 1:09 PM, William DURAND wrote:

Well done. I'll review your work soon.

To run the propel's test suite on pgsql isn't easy unfortunately but it should be better with Propel2.

Le 3 mars 2012 à 17:21, Alan Pinsteinreply@reply.github.com a écrit :

Also you need the "reverse_bookstore" table to exist in the DB for this test to run ok... I wasn't sure where to add that info since there is really no info in the README about testing pgsql.

But if as postgres you run:

create database reverse_bookstore;

That's all that is needed to run my tests.


Reply to this email directly or view it on GitHub:
#194 (comment)


Reply to this email directly or view it on GitHub:
#194 (comment)

Owner

willdurand commented Mar 5, 2012

Merged, thanks! Don't forget to port this PR on Propel2 ;)

@willdurand willdurand closed this Mar 5, 2012

Contributor

apinstein commented Mar 17, 2012

I started it, but all the Propel2 code is php53+namespaces so it wasn't as trivial a port as I'd hoped.

propelorm/Propel2#154

More to come or if you can't wait feel free to tackle it. Just the test needs to be ported.

Alan

On Mar 5, 2012, at 8:42 AM, William DURAND wrote:

Merged, thanks! Don't forget to port this PR on Propel2 ;)


Reply to this email directly or view it on GitHub:
#194 (comment)

Contributor

nnarhinen commented Mar 18, 2012

Hmm.. that original changeset was made by me. I'll try to find time to look up the original use case and fix it with unit tests this time.. I'm 99% sure this revertion will add a regression.. Btw, did you try to re-produce it using the column definition I put in the trac issue?

Contributor

apinstein commented Mar 18, 2012

For reference, this is the trac issue:

http://trac.propelorm.org/ticket/1150

I didn't try because frankly I didn't understand what your column default was. That's a syntax I don't know so I didn't feel I would be able to correctly determine if I'd handled it correctly.

Now that I have a framework for unit testing this, you should be able to add in your case trivially.

On Mar 18, 2012, at 5:42 PM, Niklas Närhinen wrote:

Hmm.. that original changeset was made by me. I'll try to find time to look up the original use case and fix it with unit tests this time.. I'm 99% sure this revertion will add a regression.. Btw, did you try to re-produce it using the column definition I put in the trac issue?


Reply to this email directly or view it on GitHub:
#194 (comment)

@willdurand willdurand referenced this pull request in propelorm/Propel2 Mar 7, 2012

Closed

Port PR #194 from Propel 1.6 #148

willdurand added a commit that referenced this pull request Sep 9, 2013

Merge pull request #194 from staabm/patch-6
Update documentation/01-installation.markdown
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment