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

__repr__ methods for classes TinyDB, Table and Query #229

Merged
merged 5 commits into from Sep 13, 2018

Conversation

@Xarvalus
Copy link
Contributor

Xarvalus commented Sep 8, 2018

Hello, considers #226.

I have added basic __repr__ implementation for mentioned classes, would like to know if you see it as sufficient and which else classes should be implemented within the same way.

You may try the reprs with:

from tinydb import database, queries

print(repr(database.TinyDB('test.json')))
print(repr(database.Table(database.JSONStorage('test.json'), 'some name')))

Fruit = queries.Query()
print(repr(Fruit.type == 'peach'))

Achieving results:

TinyDB(tables={'_default'}, tables_count=1, default_table_documents_count=0, all_tables_documents_count=['_default=0'])
Table(name='some name', total=1, storage=<tinydb.storages.JSONStorage object at 0x10212c2e8>)
QueryImpl('==', ('type',), 'peach')
@cool-RR

This comment has been minimized.

Copy link

cool-RR commented Sep 8, 2018

Thanks for volunteering to help.

The first two reprs are wrong because they use a Foo(bar) format which should only be used if the object could be reconstructed completely from Foo(Bar). I think Query can but the others can't.
So you need something like <Table name='foo' n_entries=7>

@cool-RR

This comment has been minimized.

Copy link

cool-RR commented Sep 8, 2018

Also I'm not sure that you ran your code. I suggest adding tests for these __repr__ methods.

@Xarvalus

This comment has been minimized.

Copy link
Contributor Author

Xarvalus commented Sep 8, 2018

Thanks for feedback.

I didn't add tests as __repr__ method are stripped away from test coverage, the above example is fairly easy and minimal to just one-time run with temporary script file for manual testing but if we prefer to spread more repr's across the classes I agree and will add those to tests.

I will adjust to the proper format shortly.

What about the sufficient info in __repr__s? Anything more I should add? Any thumb rule about it?

Also which classes do you prefer to also implement reprs?

@cool-RR

This comment has been minimized.

Copy link

cool-RR commented Sep 8, 2018

  1. I think that tests would be good.
  2. Looking forward to seeing your new commit.
  3. You've got sufficient information there.
  4. These three classes are great for now.
Xarvalus added 2 commits Sep 8, 2018
@Xarvalus

This comment has been minimized.

Copy link
Contributor Author

Xarvalus commented Sep 8, 2018

@cool-RR I have updated the PR, please kindly take a look

On 2.7 one of test failed, caused by https://stackoverflow.com/questions/9773121/removing-u-in-list do you suggest any best practice for keeping result the same across 2.x/3.x?

@cool-RR

This comment has been minimized.

Copy link

cool-RR commented Sep 8, 2018

Looks good to me. I'm not the project owner, so he'll need to approve before merging.

@cool-RR

This comment has been minimized.

Copy link

cool-RR commented Sep 8, 2018

@Xarvalus Xarvalus changed the title [WIP] __repr__ methods for classes TinyDB, Table and Query __repr__ methods for classes TinyDB, Table and Query Sep 9, 2018
@cool-RR

This comment has been minimized.

Copy link

cool-RR commented on 78c78ec Sep 11, 2018

Good idea using a regex. A few improvements:

  1. You can remove the casting to bool and the is True. You can just do assert re.match(pattern, repr(TinyDB(path))) and Python will deal with it :)
  2. You're escaping too many characters in your regular expression, making it harder to read. There's no need to escape these characters: _=,><
  3. Don't use + to add string literals. You can just put two string literals one after another and Python will add them up for you by default. So just remove the plus sign and you're good.

Good job!

@Xarvalus

This comment has been minimized.

Copy link
Contributor Author

Xarvalus commented Sep 11, 2018

Hey @msiemens meanwhile I have changed TinyDB's class repr test into regex one, just to match the Python 2.7's u strings issue.

Currently it passes all the checks, let know if you would like any adjustments

@Xarvalus

This comment has been minimized.

Copy link
Contributor Author

Xarvalus commented Sep 11, 2018

Oh, sorry.. I didn't see you wrote about 2 mins before me 😄 @cool-RR okay, will push your suggestions

@cool-RR

This comment has been minimized.

Copy link

cool-RR commented Sep 11, 2018

Great. Looks good to me. I'm not the project owner, so he'll need to approve before merging.

@Xarvalus

This comment has been minimized.

Copy link
Contributor Author

Xarvalus commented Sep 11, 2018

Sure 🙂 thanks for the feedback @cool-RR I'm waiting for @msiemens opinion

@msiemens msiemens merged commit b0e60b2 into msiemens:master Sep 13, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@msiemens

This comment has been minimized.

Copy link
Owner

msiemens commented Sep 13, 2018

Thanks for your work @Xarvalus, it's much appreciated 🙂

@msiemens

This comment has been minimized.

Copy link
Owner

msiemens commented Sep 13, 2018

This is now released in TinyDB v3.11.1 🙃

@cool-RR

This comment has been minimized.

Copy link

cool-RR commented Sep 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.