Improve creation of column and sort enums #210
Conversation
Cito
force-pushed
the
convert-enums-2
branch
2 times, most recently
from
36ecbfb to
c401e76
Compare
|
Wow, I love it. I was trying to fix this problem myself. I tried the approach that is in graphene-django, that is to name each enum like {column.table.name}_{column.name}. This solves 95% of the problems. But this is a much better solution. Please merge! |
- create separate enum module - create enums based on object type, not based on model - provide more customization options - split tests in different modules - adapt flask_sqlalchemy example - use conftest.py for better test fixtures
There was a problem hiding this comment.
Looks good! Great work on this @Cito 👏🏻
Btw Github PR does not handle well force-pushed commits. I'm not able to reply to some of your comments and I can't easily see the changes you just pushed. In the future it would be better to make your changes in a separate commit (unless you need to rebase your branch on master).
I noticed that it uses set() when doing the comparison, so it never really checked the order. In fact, it's not that simple to check whether no sorting happens since sqlite does not guarantee any sort order with no "order by". Usually it's the order of the primary key index, which is exactly the same as the default order. We would need to contrive something different and more complicated to check that it's really not sorting. I suggest creating a separate issue for that, though, to not delay this any further.
Can you add a TODO before merging your changes so we don't forget about it?
I wanted to squash everything into a single commit, but you're right, it's better to do that after the review when merging. In fact as I learned only now, the GitHub merge button has an option that does it automatically.
I have added one in the code. But now you need to approve again. |
There was a problem hiding this comment.
Ok great. Yes I'd like to have this repo default to auto-squash commits when merging but I don't have access. Is Syrus still the owner of this repo?
https://help.github.com/en/articles/configuring-commit-squashing-for-pull-requests
|
@jnak The graphql-python org is the owner, and the governors team can admin, i.e. I can change that. But changing the default can only be done by disabling other options. Not sure if we should do that. |
|
@Cito I can't think of a case where we would want multiple commits for a PR instead of one single squashed commit. It makes the git history much cleaner and easier to navigate. Can we disable all other options for now? We can always re-enable them if we realize we're not happy with it. |
|
Ok, deactivated the other options now. |
This is an implementation of what has been discussed in #208.
When getting sort enums, you can now specify that you only want to sort certain columns or only the indexed columns.