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

Provide a way to specify a non-default table name when creating TinyD… #98

Merged
merged 3 commits into from Apr 20, 2016

Conversation

3 participants
@deemson
Contributor

deemson commented Apr 20, 2016

…B instance.

@msiemens

This comment has been minimized.

Show comment
Hide comment
@msiemens

msiemens Apr 20, 2016

Owner

Looks reasonable, thank you! While we're at it, could you also add a short test in tests/test_tinydb.py?

Owner

msiemens commented Apr 20, 2016

Looks reasonable, thank you! While we're at it, could you also add a short test in tests/test_tinydb.py?

@deemson

This comment has been minimized.

Show comment
Hide comment
@deemson

deemson Apr 20, 2016

Contributor

Sure.

Contributor

deemson commented Apr 20, 2016

Sure.

Provide a way to specify a non-default table name when creating TinyD…
…B instance:

moved kwargs retrieval to the top of init method and added a tiny test.
@deemson

This comment has been minimized.

Show comment
Hide comment
@deemson

deemson Apr 20, 2016

Contributor

moved kwargs pops to the top of init because storage instances don't recognize 'table' keyword

Contributor

deemson commented Apr 20, 2016

moved kwargs pops to the top of init because storage instances don't recognize 'table' keyword

@msiemens

This comment has been minimized.

Show comment
Hide comment
@msiemens

msiemens Apr 20, 2016

Owner

I must have missed it in the first commit, but you introduced a new kwarg parameter to Table.__init__(), right? I'm conflicted on that because technically that's a breaking change (storages cannot use the table keyword anymore). But on the other hand I don't think this will break anyone's code as writing a storage that takes a a table kwarg isn't a good idea in the first place.

Owner

msiemens commented Apr 20, 2016

I must have missed it in the first commit, but you introduced a new kwarg parameter to Table.__init__(), right? I'm conflicted on that because technically that's a breaking change (storages cannot use the table keyword anymore). But on the other hand I don't think this will break anyone's code as writing a storage that takes a a table kwarg isn't a good idea in the first place.

@deemson

This comment has been minimized.

Show comment
Hide comment
@deemson

deemson Apr 20, 2016

Contributor

I've introduced a new TinyDB.__init()__ kwarg and it won't be passed to storage, it is being popped out of kwargs dictionary right before the creation of storage instance.

Contributor

deemson commented Apr 20, 2016

I've introduced a new TinyDB.__init()__ kwarg and it won't be passed to storage, it is being popped out of kwargs dictionary right before the creation of storage instance.

@eugene-eeo

This comment has been minimized.

Show comment
Hide comment
@eugene-eeo

eugene-eeo Apr 20, 2016

Contributor

May be a matter of taste but I think using a longer keyword argument like default_table would be preferable here as it better reflects the purpose of the parameter, and leaves less room for confusion in the future.

As an aside, I don't see why one would want to override the default table. Don't get me wrong, I like how you're handling it elegantly here, but I don't see why the application would care about the default table name.

Contributor

eugene-eeo commented Apr 20, 2016

May be a matter of taste but I think using a longer keyword argument like default_table would be preferable here as it better reflects the purpose of the parameter, and leaves less room for confusion in the future.

As an aside, I don't see why one would want to override the default table. Don't get me wrong, I like how you're handling it elegantly here, but I don't see why the application would care about the default table name.

@deemson

This comment has been minimized.

Show comment
Hide comment
@deemson

deemson Apr 20, 2016

Contributor

I cannot argue with this type of reasoning, this is a matter of taste. I just don't want to see "_default" keyword in my storage files. I find it ugly.

Agreed on the parameter naming, though.

Contributor

deemson commented Apr 20, 2016

I cannot argue with this type of reasoning, this is a matter of taste. I just don't want to see "_default" keyword in my storage files. I find it ugly.

Agreed on the parameter naming, though.

Provide a way to specify a non-default table name when creating TinyD…
…B instance: changed the default table param name from 'table' to 'default_table'
@msiemens

This comment has been minimized.

Show comment
Hide comment
@msiemens

msiemens Apr 20, 2016

Owner

default_table sounds good to me. The name of the default table indeed is a matter of taste and I see no problem with allowing to rename it.

Owner

msiemens commented Apr 20, 2016

default_table sounds good to me. The name of the default table indeed is a matter of taste and I see no problem with allowing to rename it.

@msiemens msiemens merged commit 27862f5 into msiemens:master Apr 20, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 99.734%
Details
@msiemens

This comment has been minimized.

Show comment
Hide comment
@msiemens

msiemens Apr 20, 2016

Owner

Merged! I hope to get a new release of TinyDB ready by the end of this week. Thanks everyone!

Owner

msiemens commented Apr 20, 2016

Merged! I hope to get a new release of TinyDB ready by the end of this week. Thanks everyone!

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