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

Support to dump and load of postgresql schemas #1450

Closed
wants to merge 8 commits into from

Conversation

lucasts
Copy link
Contributor

@lucasts lucasts commented Jun 1, 2011

cc: @jonleighton, @tenderlove

As of PR #1410, this change try to respect table with qualified schemas in postgres

now you can do something like this on a migration file:

    create_table 'myschema.mytable' do |t|  
      t.string :column          , :null => false, :limit => 20
      t.timestamps  
    end

something I want to work soon is to create schemas in migrations without breaking dump and create tasks(that will also break test tasks)

@jonleighton
Copy link
Member

Hiya,

This looks identical to what I reverted, which broke the tests.

The reason it broke is because this line is checking for 'schema_things', but after your change the entry is 'rails_pg_schema_user1.schema_things', hence the assertion fails.

I'm not sure what the 'right' behaviour is here so I will let @tenderlove comment as he is more familiar with the adapters code.

Jon

@lucasts
Copy link
Contributor Author

lucasts commented Jun 1, 2011

Hi Jon,

Yes, almost the same.
The fix is at line 663 https://github.com/rails/rails/pull/1450/files#L0R663
Now I'm checking if the schemaname is a authorization schema for the current user.

I ran the test that you're pointing and it pass now.

Did you run it with this new patch?

@jonleighton
Copy link
Member

Ok, I didn't notice the difference. I will let @tenderlove check it over.

Just on a code quality point, I would prefer if you hadn't changed the if/else statement into a ternary.

@lucasts
Copy link
Contributor Author

lucasts commented Jun 1, 2011

Ok, lets wait for him.
About your point, I stopped for 5min to think about it before rewrote as ternary.
Is that preference (if/else over ternary) a ruby pattern?

thanks for the tip.

@jonleighton
Copy link
Member

There's no hard and fast rule, but if an if/else is at all complicated, then I favour a full if/else rather than ternary. I find it easier to read. Especially in this case where your condition includes an == in addition to the = in the assignment.

@lucasts
Copy link
Contributor Author

lucasts commented Jun 1, 2011

Agreed @jonleighton
The multiple '=' chars in the line may lead to misunderstandings in the future.

@lucasts
Copy link
Contributor Author

lucasts commented Jun 8, 2011

@tenderlove,
As the "same" PR was merged before, but reverted due to a little break, that I fixed now

Is anything else left to merge this?

Cheers
-lucas

@isaacsanders
Copy link
Contributor

Is this still an issue @lucasts?

@rafaelfranca
Copy link
Member

This pull request cannot be automatically merged. If you still want to merge it, please rebase against the master. If no, please close it.

Thanks.

@lucasts
Copy link
Contributor Author

lucasts commented May 3, 2012

I'll close this PR and investigate the current status of the root problem.

If I found something that need fix I will create a new PR with uptated code. Since last interaction I found other issues with postgresql schema support that need to be fixed too.

@lucasts lucasts closed this May 3, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants