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

Add @withoutGlobalScopes directive #2577

Merged
merged 15 commits into from
Jul 8, 2024

Conversation

saeed-rostami
Copy link
Contributor

add @withoutGlobalScopes directive which works on argument to ignore global scopes that are defined in the model.

type Query {
posts
(
allPosts (Bool @withoutGlobalScopes (names : ["scheduled"])
)
}

@saeed-rostami
Copy link
Contributor Author

Hello
should I add the directive description in the docs md file?

@spawnia
Copy link
Collaborator

spawnia commented Jun 30, 2024

Hello should I add the directive description in the docs md file?

Eventually, yeah. Let's make sure it is properly described first.

src/Schema/Directives/WithoutGlobalScopesDirective.php Outdated Show resolved Hide resolved
src/Schema/Directives/WithoutGlobalScopesDirective.php Outdated Show resolved Hide resolved
src/Schema/Directives/WithoutGlobalScopesDirective.php Outdated Show resolved Hide resolved
src/Schema/Directives/WithoutGlobalScopesDirective.php Outdated Show resolved Hide resolved
src/Schema/Directives/WithoutGlobalScopesDirective.php Outdated Show resolved Hide resolved
tests/Utils/Models/Episode.php Outdated Show resolved Hide resolved
tests/Utils/Models/Episode.php Outdated Show resolved Hide resolved
tests/database/factories/EpisodeFactory.php Outdated Show resolved Hide resolved
tests/Utils/Models/Episode.php Outdated Show resolved Hide resolved
@spawnia spawnia added the enhancement A feature or improvement label Jun 30, 2024
saeed-rostami

This comment was marked as resolved.

@saeed-rostami
Copy link
Contributor Author

Thanks again, new commit pushed ...check the doc file please

Copy link
Collaborator

@spawnia spawnia left a comment

Choose a reason for hiding this comment

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

Please let me know when all comments are addressed.

docs/6/api-reference/directives.md Outdated Show resolved Hide resolved
src/Schema/Directives/WithoutGlobalScopesDirective.php Outdated Show resolved Hide resolved
tests/database/factories/EpisodeFactory.php Outdated Show resolved Hide resolved
@spawnia
Copy link
Collaborator

spawnia commented Jul 2, 2024

There is a typo in the title, and tests are failing.

@spawnia spawnia marked this pull request as draft July 2, 2024 10:15
@saeed-rostami
Copy link
Contributor Author

There is a typo in the title, and tests are failing.

Can you give me more details about the test error?

@spawnia
Copy link
Collaborator

spawnia commented Jul 3, 2024

There is a typo in the title, and tests are failing.

Can you give me more details about the test error?

You can look at the results in https://github.com/nuwave/lighthouse/actions/runs/9746838503.

@saeed-rostami
Copy link
Contributor Author

Ok..., I pushed another commit...hope will be succeed

@spawnia
Copy link
Collaborator

spawnia commented Jul 3, 2024

Ok..., I pushed another commit...hope will be succeed

You don't have to hope, you can run the tests locally. See https://github.com/nuwave/lighthouse/blob/master/CONTRIBUTING.md.

Please let me know when every comment has been addressed and CI passes.

@saeed-rostami
Copy link
Contributor Author

Ok..., I pushed another commit...hope will be succeed

You don't have to hope, you can run the tests locally. See https://github.com/nuwave/lighthouse/blob/master/CONTRIBUTING.md.

Please let me know when every comment has been addressed and CI passes.
I'm living in Iran...docker is sanctioned and the internet situation is bad ... but I will try anyway
thanks

@saeed-rostami
Copy link
Contributor Author

Finally, I tested locally and both tests were successful.

@spawnia
Copy link
Collaborator

spawnia commented Jul 7, 2024

Finally, I tested locally and both tests were successful.

There are still plenty of unresolved discussions. Please address them all.

@saeed-rostami
Copy link
Contributor Author

Finally, I tested locally and both tests were successful.

There are still plenty of unresolved discussions. Please address them all.

please check them

@saeed-rostami
Copy link
Contributor Author

NEW COMMIT PUSHED

@spawnia
Copy link
Collaborator

spawnia commented Jul 8, 2024

NEW COMMIT PUSHED

I am a bit irritated by your use of ALL CAPS.

Anyways, thanks for making the requested changes. I think I failed to communicate how I would like certain things to be done very specifically. The intent of your changes is clear, so I am going to go ahead and make smaller changes, I think that will be more efficient than going back and forth.

@spawnia spawnia changed the title ADD @witohutGlobalScopesDirective and test Add @withoutGlobalScopes directive Jul 8, 2024
@spawnia spawnia marked this pull request as ready for review July 8, 2024 09:38
@spawnia spawnia merged commit f60fb9f into nuwave:master Jul 8, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants