search

jgaskins / perpetuity-postgres

Postgres adapter for Perpetuity
MIT License
10 stars 7 forks source link

Remove VARCHAR & refactor into hash lookup. #12

Closed acook closed 10 years ago

acook commented 10 years ago

Removing VARCHAR support from Attribute and converting sql_type method into a Hash lookup.

I'm tempted to hide the constant in a seperate more generic method, but this required less code.

jgaskins commented 10 years ago

What output do you get when you just run rspec (or bin/rspec if you're using binstubs)?

acook commented 10 years ago

Okay I figured it out. I need to do this first:

createdb perpetuity_gem_test

Now I can run tests.

Also don't merge this until I fix the the tests. :fearful:

acook commented 10 years ago

Okay, tests pass here and I changed it to a Hash lookup since I originally misunderstood the purpose of the code.

Stylistically, do you prefer a different way of declaring the default 'JSON' value?

acook commented 10 years ago

I don't see VARCHAR anywhere else. Is there any additional special code around it?

I'm also tempted to define constants for each DB type, but I'll wait for that until someone has played around with Coercible.

jgaskins commented 10 years ago

Okay I figured it out. I need to do this first: createdb perpetuity_gem_test

I'm surprised at that. I dropdb all the time to make sure I'm starting from a clean slate and the adapter recreates it for me. Do you not have a db with your username on your psql installation?

If anything, this lets me know I need to handle this situation better.

I don't see VARCHAR anywhere else. Is there any additional special code around it?

Nah, it was only there if someone specified max_length, I believe. It was originally an idea to let people use VARCHAR if they wanted it, but since there's no reason to (it's apparently only there for SQL compliance) I don't see a reason to support it. If people want to limit the length of their strings, this should be done in the domain model.

I'm also tempted to define constants for each DB type, but I'll wait for that until someone has played around with Coercible.

I've thought the same thing. Or at least have something better.

Stylistically, do you prefer a different way of declaring the default 'JSON' value?

I'm pretty partial to the default value/block: Hash.new('JSON') or Hash.new { 'JSON' }. Unfortunately, you can't initialize it at the same time. Maybe something like this?

SQL_TYPE_MAP = Hash.new('JSON').merge(String => 'TEXT',
                                      Integer => 'BIGINT',
                                      # ...

I've never had to initialize a hash and provide a default at the same time, but I'm surprised Ruby doesn't allow it. I'm okay with it as it is, though.

If your tests are passing I can go ahead and merge it in. Also, shit, I need to get Travis CI hooked up on the adapter repos.

acook commented 10 years ago

I'm surprised at that. I dropdb all the time to make sure I'm starting from a clean slate and the adapter recreates it for me. Do you not have a db with your username on your psql installation?

Ah, no I don't. I have a postgres user instead. I suppose I could just create a DB for myself.

Nah, it was only there if someone specified max_length, I believe. It was originally an idea to let people use VARCHAR if they wanted it, but since there's no reason to (it's apparently only there for SQL compliance) I don't see a reason to support it. If people want to limit the length of their strings, this should be done in the domain model.

I was wondering where the VARCHAR(40) came from, was that just generated by the DB?

If your tests are passing I can go ahead and merge it in.

They pass here. :+1:

Also, shit, I need to get Travis CI hooked up on the adapter repos.

Yes I was going to suggest that. I almost did it myself but then I figured it'd be more of a hassle because I only have access to my own fork.

Whats the best way to reach you with Perpetuity questions? Here on GitHub, IRC, or elsewhere?

acook commented 10 years ago

Not to bug you, just wondering if I should :)

acook commented 10 years ago

It looks like you've removed the VARCHAR feature last week also, seems you found a hash parameter that I missed as well. Do we still need this? Should rebase it?

jgaskins commented 10 years ago

I didn't realize I'd actually committed that. I played with the idea before (I play with a lot of ideas locally that never make it into the repo), but I thought I'd gotten rid of the change. Huh.

Crap, I'm sorry, I wasn't trying to step on your work. I also thought I'd merged this PR in, so I was confused when I saw the "Do we still need this?" in the notification e-mail. I've been scatterbrained as hell lately (just got engaged last weekend, so most of my brainpower went into planning that ;-)).

Anyway, I still like the hash conversion, so I'd like to get that in. Could you cherry-pick it into another PR or maybe rebase -i and remove the VARCHAR commits?

acook commented 10 years ago

No worries, I went to look at the commits and was a little surprised is all. I'll make the changes and let you know.

:: sent from my android phone :: On Mar 4, 2014 1:46 PM, "Jamie Gaskins" notifications@github.com wrote:

I didn't realize I'd actually committed that. I played with the idea before (I play with a lot of ideas locally that never make it into the repo), but I thought I'd gotten rid of the change. Huh.

Crap, I'm sorry, I wasn't trying to step on your work. I also thought I'd merged this PR in, so I was confused when I saw the "Do we still need this?" in the notification e-mail. I've been scatterbrained as hell lately (just got engaged last weekend, so most of my brainpower went into planning that ;-)).

Anyway, I still like the hash conversion, so I'd like to get that in. Could you cherry-pick it into another PR or maybe rebase -i and remove the VARCHAR commits?

Reply to this email directly or view it on GitHubhttps://github.com/jgaskins/perpetuity-postgres/pull/12#issuecomment-36665566 .

acook commented 10 years ago

Closing this in favor of the new PR.