Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Convert simple_search_list/simple_search_list_txn to return tuples. #16434

Merged
merged 8 commits into from Oct 10, 2023

Conversation

clokep
Copy link
Contributor

@clokep clokep commented Oct 5, 2023

More removals of cursor_to_dict. This one might be controversial since it just manually creates the dict at the REST layer.

I assert that this is better because it is explicit about the information we return to clients, as opposed to choosing from the database layer.

It also does a couple of other simplifications while I was there.

See #16431.

@clokep clokep marked this pull request as ready for review October 5, 2023 15:08
@clokep clokep requested a review from a team as a code owner October 5, 2023 15:08
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with the change at large, but I'm not sure what we should do about the boolean-but-ints-on-sqlite and nullable-but-hopefully-not columns.

@clokep
Copy link
Contributor Author

clokep commented Oct 6, 2023

Also filed #16443 about there being no docs for this endpoint.

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but complement is failing in a way I don't recognise. Is it a regression?

@clokep
Copy link
Contributor Author

clokep commented Oct 10, 2023

Looks good, but complement is failing in a way I don't recognise. Is it a regression?

Seems to be failing on develop in the same way: https://github.com/matrix-org/synapse/actions/runs/6469190463/job/17562829355

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, happy for this to land. I'll try to investigate develop when I can!

@clokep clokep merged commit f1e4301 into develop Oct 10, 2023
34 of 37 checks passed
@clokep clokep deleted the clokep/axe-cursor-to-dict-4 branch October 10, 2023 16: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.

None yet

2 participants