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

Develop/exclude excludeds #329

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

engAmirEng
Copy link
Contributor

@engAmirEng engAmirEng commented May 15, 2023

The new DefaultField added, if user wants to only expose some of the fields that are implemented by grapple instead of all of them then they should wrap them in this class, otherwise everything is going to work with no breaking changes and all of the grapple's fields are exposed(as they were before)

…eryable

-if adding it to Meta.exclude works then do it, if not then return a dummy data
@zerolab
Copy link
Member

zerolab commented May 15, 2023

There are a number "core" fields for Wagtail Page-derived models which are provided by the interface and I would argue are useful.

If the goal is to return minimum information, off the top of my head, perhaps we either:
a) add a setting (something like INCLUDE_DEFINED_FIELDS_ONLY with corresponding notes in docs
b) introduce a graphql_only_fields attribute

Need to sleep on it. cc @dopry

@engAmirEng
Copy link
Contributor Author

engAmirEng commented May 15, 2023

@zerolab Just to make sure you got my approach,
I introduced a new class (DefaultField), if user wants to include grapple implemented fields selectively, then wraps them inside this class, if user wants to include all grapple's implemented fields then it's all ok with no breaking changes

@zerolab
Copy link
Member

zerolab commented May 15, 2023

Ah, thank you. I did indeed miss that when I had a cursory look. In the future, could you please:

  1. include this type of detail in the PR description
  2. add corresponding code comments. (aside: I know the code is not consistently commented, but we're chipping away at that)

Now, if we commit to going down this route, we need to follow a 2 release deprecation process similar to how core or Django does it (ref: https://docs.wagtail.org/en/stable/contributing/release_process.html#deprecation-policy) as this would result in backwards incompatible changes.

I have more time on Friday and will look more in detail.

@engAmirEng
Copy link
Contributor Author

@zerolab you are right, I should've describe the details in the PR
I was excited to see if the general approach is considerable

engAmirEng added a commit to engAmirEng/wagtail-grapple that referenced this pull request May 16, 2023
@dopry
Copy link
Collaborator

dopry commented May 20, 2023

@engAmirEng I don't really follow the changes here. I haven't dove deep into the automatic schema generation. I'm not sure what a DefaultField is or why I would want to use it. I don't understand the use case for this change. Could you update the OP to better explain your goals with these changes and how the implementation works?

@engAmirEng
Copy link
Contributor Author

@dopry Look at #330 and you'll see that I had to skip a test, try to understand why the test is failing

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

Successfully merging this pull request may close these issues.

3 participants