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

Not require explicitly set ordering in DjangoConnectionField #1518

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

kiendang
Copy link
Collaborator

I'm working on fixing #1516, probably by setting ordering = ["pk"] by default in DjangoConnectionField class Meta.

However I don't find any reference of ordering in both this or Graphene's code base. Does anyone have any idea? Or is setting ordering actually not doing anything? @sjdemartini @tcleonard

@kiendang kiendang linked an issue Apr 11, 2024 that may be closed by this pull request
@gsvr
Copy link

gsvr commented Apr 15, 2024

@kiendang It looks like you're getting stuck at the same place I did - there doesn't seem to be an easy way to define ordering in a Meta class somewhere. The only ordering method I could find in the docs was using an OrderingFilter, but setting a default with that doesn't look easily viable. I would suggest opening another feature issue to implement default ordering so this PR can be wrapped up :)

@kiendang
Copy link
Collaborator Author

kiendang commented Apr 15, 2024

@gsvr my problem is that I don't know if setting

class Meta
    ordering = ["pk"]

actually does anything because I can't seem to find any code that references ordering in this or Graphene code base.

If that is the case, we should just revert #1495.

In case that ordering actually works, setting a default ordering is doable, but even in this case I'm fine with reverting #1495 first then implementing that later.

@gsvr
Copy link

gsvr commented Apr 15, 2024

@kiendang Yeah, I also searched around, and I don't think setting ordering in class Meta actually does anything. I couldn't find any usage of that in the docs or the code.

@kiendang kiendang marked this pull request as ready for review April 16, 2024 04:03
Copy link
Collaborator

@sjdemartini sjdemartini left a comment

Choose a reason for hiding this comment

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

lgtm to revert, thanks for handling this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforcing model default ordering (#1495) may have been overzealous
4 participants