Skip to content
This repository was archived by the owner on May 13, 2023. It is now read-only.

Conversation

kaboc
Copy link
Contributor

@kaboc kaboc commented Jun 25, 2021

What kind of change does this PR introduce?

Align the functionality of order() with that of postgres-js by adding the same new feature.
This closes #30 .

What is the current behavior?

Sorting by multiple columns is not supported.

What is the new behavior?

Multiple order()s can be used together to sort records by multiple columns. See #30 for details.

Additional context

It looks like transforms_test.dart does not follow the lines_longer_than_80_chars linter rule, so I didn't run dart format on the file.


I fixed one of the existing tests too.

test('embedded order', () async {
final res = await postgrest
.from('users')
.select('messages(*)')
.order('channel_id', foreignTable: 'messages')
.execute();
expect(res.data[0]['messages'].length, 2);
expect(res.data[1]['messages'].length, 0);
expect(res.data[2]['messages'].length, 0);
expect(res.data[3]['messages'].length, 0);
});

This is the test code before my fix. It only checked the number of results without checking the order, which was not appropriate as a test for sorting.


I ignored the equivalent tests in postgres-js because they don't make sure that sort results are as expected.

https://github.com/supabase/postgrest-js/blob/e5f5dfae7da22f4ff63c11c124d65d0699b7b890/test/transforms.ts#L10-L17
https://github.com/supabase/postgrest-js/blob/e5f5dfae7da22f4ff63c11c124d65d0699b7b890/test/resource-embedding.ts#L41-L48

The messages table has two records with the same username ("supabot"), so sorting by the column does not mean the expected behaviour is correctly tested. I think these should be fixed.

@kaboc
Copy link
Contributor Author

kaboc commented Jun 25, 2021

Oh, I didn't notice some other tests had altered the data in the database before I started working on this, and so I wrote the test that depended on the affected data. I'll improve the failed test to use another column that isn't altered.

I don't think it's very good that the results of some tests differ depending on the execution order of tests. I recommend improving it at some point.

@dshukertjr
Copy link
Member

dshukertjr commented Jun 25, 2021

Thank you for the amazing work @kaboc !

@phamhieu about the number of characters per line thing, I have noticed it before, but we used to go with 100 characters per line, correct? Did we recently change it to 80 per line? Should we just format everything at 80 per line? Otherwise, it looks good to me!

@dshukertjr
Copy link
Member

Pham and I talked about letters per line problem in a separate thread, and will take care of it in a separate PR! Thank you @kaboc for an amazing work 🎉🎉🎉

@dshukertjr dshukertjr merged commit 5cf766c into supabase:master Jun 26, 2021
@kaboc kaboc deleted the multiple_orders branch June 26, 2021 12:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sorting by multiple columns

2 participants