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

Move the rest_filters reserved names to a setting #384

Merged
merged 1 commit into from
May 16, 2024

Conversation

AlanCoding
Copy link
Member

My 2 cents here - this code was really just copied over from AWX. There is a process that code needs to go through in order to generalize it so that it's just abstract, for use via a library. This never happened for at least this part of rest_filters. Since it came up, I wanted to go ahead and get it done.

@AlanCoding AlanCoding marked this pull request as ready for review May 10, 2024 20:45
Copy link
Member

@relrod relrod left a comment

Choose a reason for hiding this comment

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

I might suggest renaming the settings variable to indicate that those are always added even if a view overrides them, if that's purely additive.

But overall LGTM.

Copy link
Member

@john-westcott-iv john-westcott-iv left a comment

Choose a reason for hiding this comment

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

Mostly minor comments. If it went in as built it wouldn't be the end of my world.

Copy link

sonarcloud bot commented May 16, 2024

@AlanCoding AlanCoding merged commit 05c888f into ansible:devel May 16, 2024
8 checks passed
thedoubl3j pushed a commit to thedoubl3j/django-ansible-base that referenced this pull request Jun 18, 2024
My 2 cents here - this code was really just copied over from AWX. There
is a process that code needs to go through in order to generalize it so
that it's just abstract, for use via a library. This never happened for
at least this part of rest_filters. Since it came up, I wanted to go
ahead and get it done.
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.

None yet

4 participants