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

Rails db console improvements #6150

Merged
merged 2 commits into from May 22, 2012

Conversation

Projects
None yet
3 participants
@avakhov
Contributor

avakhov commented May 4, 2012

The code in active_record/connection_adapters/sqlite3_adapter.rb is slightly dublicated by rails/commands/dbconsole.rb#L76-83.

I don't think it's very bad, but I suggest to improve db console a little more. In #6012 I covered db console with tests and we can now improve it a little.

The first commit - abort if we execute rails db on memory dtabase. The second - to use rails root expand path as here: connection_adapters/sqlite3_adapter.rb#L21.

The first improvment I found only investigating code, but the second is from real life. I developed gem and executed dummy server ./spec/dummy/scripts/rails server. It worked well. But ./spec/dummy/scripts/rails db failed with not resolved path to db file.

@avakhov

This comment has been minimized.

Show comment
Hide comment
@avakhov

avakhov May 4, 2012

Contributor

The third commit fixes rails db -h command. It should print usage but really doesn't. It because ruby try to use '-h' as short version of '--header' by default - https://github.com/ruby/ruby/blob/trunk/lib/optparse.rb#L1372-1381. 4c872c0 didn't resolve the problem. \cc @vijaydev

Contributor

avakhov commented May 4, 2012

The third commit fixes rails db -h command. It should print usage but really doesn't. It because ruby try to use '-h' as short version of '--header' by default - https://github.com/ruby/ruby/blob/trunk/lib/optparse.rb#L1372-1381. 4c872c0 didn't resolve the problem. \cc @vijaydev

@avakhov

This comment has been minimized.

Show comment
Hide comment
@avakhov

avakhov May 7, 2012

Contributor

Rebased and resolved conflicts after #5910

Contributor

avakhov commented May 7, 2012

Rebased and resolved conflicts after #5910

@avakhov

This comment has been minimized.

Show comment
Hide comment
@avakhov

avakhov May 12, 2012

Contributor

Hello! Review somebody this please. The second commit (executing rails db from not rails root) is pretty usefull for gem development. I rebased and retest just only.

Contributor

avakhov commented May 12, 2012

Hello! Review somebody this please. The second commit (executing rails db from not rails root) is pretty usefull for gem development. I rebased and retest just only.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 12, 2012

Member

Seems good to me.

cc/ @jeremy

Member

rafaelfranca commented May 12, 2012

Seems good to me.

cc/ @jeremy

@avakhov

This comment has been minimized.

Show comment
Hide comment
@avakhov

avakhov May 19, 2012

Contributor

rebased & retested

Contributor

avakhov commented May 19, 2012

rebased & retested

@drogus

View changes

Show outdated Hide outdated railties/lib/rails/commands/dbconsole.rb

@ghost ghost assigned drogus May 20, 2012

@avakhov

This comment has been minimized.

Show comment
Hide comment
@avakhov

avakhov May 21, 2012

Contributor

Piotr, thanks for the issue. I agree with you and remove this commit with memory database. The last two commits I found in my practice, the commit with memory db I suggested investigating the sources. It appears that It was not so well fit.

Frankly speaking it's very weird to use :memory: in development (or production :). It's usefull only for test mode (on my box AR tests are passed 2 times faster with sqlite3_mem adapter). May be we should add the notice in guides. Or do nothing :)

Contributor

avakhov commented May 21, 2012

Piotr, thanks for the issue. I agree with you and remove this commit with memory database. The last two commits I found in my practice, the commit with memory db I suggested investigating the sources. It appears that It was not so well fit.

Frankly speaking it's very weird to use :memory: in development (or production :). It's usefull only for test mode (on my box AR tests are passed 2 times faster with sqlite3_mem adapter). May be we should add the notice in guides. Or do nothing :)

@drogus

This comment has been minimized.

Show comment
Hide comment
@drogus

drogus May 22, 2012

Member

@avakhov thanks for the answer. Could you also format commit messages according to what I added to contribution guide here: ae8b09c ?

Member

drogus commented May 22, 2012

@avakhov thanks for the answer. Could you also format commit messages according to what I added to contribution guide here: ae8b09c ?

avakhov added some commits May 4, 2012

Use relative path to sqlite3 db in `rails db` command
Rails uses sqlit3 db file with a path relative to the rails root. It
allows to execute server not from rails root only. For example you
can fire `./spec/dummy/script/rails s` to start dummy application
server if you develop some engine gem.

Now the `rails db` command uses relative paths also and you can explore
your dummy db via `./spec/dummy/script/rails db` command.
Fix `rails db -h` and cosmetic fixes in usage banners
Ruby tries to use '-h' as short version of '--header' by default
https://github.com/ruby/ruby/blob/trunk/lib/optparse.rb#L1372-1381.
To force `rails db -h` prints an usage message we should add the `-h`
options explicitly.
@avakhov

This comment has been minimized.

Show comment
Hide comment
@avakhov

avakhov May 22, 2012

Contributor

@drogus I cutted a little summaries and added descriptions to the both commits.

I would like to know your opinion about applying pull requests with rebase instead of merge. I think it would be great to have linear git history. We could work more with git bisect and decrease errors like #5359. It's a pity that github doesn't have such feature, isn't it?

Contributor

avakhov commented May 22, 2012

@drogus I cutted a little summaries and added descriptions to the both commits.

I would like to know your opinion about applying pull requests with rebase instead of merge. I think it would be great to have linear git history. We could work more with git bisect and decrease errors like #5359. It's a pity that github doesn't have such feature, isn't it?

@drogus

This comment has been minimized.

Show comment
Hide comment
@drogus

drogus May 22, 2012

Member

@avakhov thanks!

Regarding merges, I also raised that concerned some time ago, specifically for merges with one small commit as it seems overkill in such situations. However @josevalim pointed out that with merge commits we have much better reference to pull requests, otherwise we could not easily track the source of the commit. It's not always needed, but is often useful.

Also, you can browse git history with --no-merges option.

Member

drogus commented May 22, 2012

@avakhov thanks!

Regarding merges, I also raised that concerned some time ago, specifically for merges with one small commit as it seems overkill in such situations. However @josevalim pointed out that with merge commits we have much better reference to pull requests, otherwise we could not easily track the source of the commit. It's not always needed, but is often useful.

Also, you can browse git history with --no-merges option.

drogus added a commit that referenced this pull request May 22, 2012

@drogus drogus merged commit d22859e into rails:master May 22, 2012

@avakhov

This comment has been minimized.

Show comment
Hide comment
@avakhov

avakhov May 22, 2012

Contributor

Yes, I take your point about merge vs rebase. It's profound.

Contributor

avakhov commented May 22, 2012

Yes, I take your point about merge vs rebase. It's profound.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment