fixing table config and adding tests #102
Conversation
|
This pull request introduces 1 alert when merging 69b0a86 into 8fa1847 - view on LGTM.com new alerts:
|
|
@dantownsend These tests should pass when you merge the changes in Piccolo and Piccolo APIs. Locally it's OK. |
tests/test_endpoints.py
Outdated
| class TableA(Table): | ||
| class Post(Table): | ||
| name = Varchar(length=100) | ||
| content = Text() | ||
| created = Timestamp() | ||
|
|
||
|
|
||
| class TableB(Table): | ||
| table_a = ForeignKey(TableA) | ||
|
|
||
|
|
||
| class TableC(Table): | ||
| table_b = ForeignKey(TableB) | ||
| class TestTableConfig(TestCase): | ||
| def test_visible_columns(self): | ||
| post_table = TableConfig( | ||
| table_class=Post, | ||
| visible_columns=[Post._meta.primary_key, Post.name], | ||
| ) | ||
| self.assertEqual( | ||
| post_table.get_visible_column_names(), | ||
| ("id", "name"), | ||
| ) | ||
|
|
||
| def test_exclude_visible_columns(self): | ||
| post_table = TableConfig( | ||
| table_class=Post, | ||
| exclude_visible_columns=[Post._meta.primary_key, Post.name], | ||
| ) | ||
| self.assertEqual( | ||
| tuple(i._meta.name for i in post_table.get_visible_columns()), | ||
| ("content", "created"), | ||
| ) | ||
|
|
||
| class TestGetAllTables(TestCase): | ||
| def test_all_returned(self): | ||
| tables = get_all_tables([TableC]) | ||
| self.assertEqual(tables, [TableC, TableB, TableA]) |
There was a problem hiding this comment.
We should add this test back. get_all_tables is quite complex now - we probably need some more tests which pass in a combination of Table and TableConfig to make sure it works correctly.
There was a problem hiding this comment.
I removed this test because with this new code (I'm not saying it's a good) get_references no longer returns reference table because there was duplication in the output list. We can completly remove get_references function and the result will be the same and get_all_tables return combination of Table and TableConfig without problems. Test like this proves it
class TestGetAllTables(TestCase):
def test_all_returned(self):
tables = get_all_tables(
[TableConfig(TableC), TableB, TableConfig(TableA)]
)
self.assertEqual(
tables,
[TableConfig(TableC), TableB, TableConfig(TableA)],
)
self.assertNotEqual(
tables,
[TableConfig(TableC), TableB, TableA],
)There was a problem hiding this comment.
Yeah, I see what you mean. I refactored the code slightly - there shouldn't be any duplicate tables any more.
There was a problem hiding this comment.
I confirm that latest commit fixed duplication problem.
|
@sinisaos Thanks for this. I'll try it out properly later today. |
|
@sinisaos I think this is good to go now, besides docs. I need to release Piccolo and Piccolo API first, so we can get these tests passing. Does it look good to you? |
|
@dantownsend This looks and works great. Tests should pass when you merge the changes in Piccolo and Piccolo API. Locally it's OK. If I write docs you will probably have to correct a lot because I am not a native English speaker, so it is better for you to write them, if it's not a problem. |
|
@sinisaos Thanks. I'll write the docs in a bit. |
Codecov Report
@@ Coverage Diff @@
## master #102 +/- ##
==========================================
+ Coverage 97.90% 98.32% +0.42%
==========================================
Files 3 3
Lines 143 179 +36
==========================================
+ Hits 140 176 +36
Misses 3 3
Continue to review full report at Codecov.
|
|
@sinisaos I think it's done now! Take a look at the docs if you get a chance to check it makes sense. |
|
@dantownsend This is a great new feature and docs are great and nicely explain how to use this new feature. |
@dantownsend I made this PR with code working and added tests, and now you can add commits here (avoiding long comments in the conversation).